Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560322 > unrolled thread

[PATCH] usb: gadget: configfs: Fix KASAN use-after-free

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: gadget: configfs: Fix KASAN use-after-free Jim Lin <jilin@nvidia.com> - 2017-01-17 09:10 +0100
    Re: [PATCH] usb: gadget: configfs: Fix KASAN use-after-free Felipe Balbi <balbi@kernel.org> - 2017-01-17 09:20 +0100
      Re: [PATCH] usb: gadget: configfs: Fix KASAN use-after-free Greg KH <greg@kroah.com> - 2017-01-17 10:30 +0100
        Re: [PATCH] usb: gadget: configfs: Fix KASAN use-after-free Felipe Balbi <balbi@kernel.org> - 2017-01-17 10:40 +0100
      Re: [PATCH] usb: gadget: configfs: Fix KASAN use-after-free Jim Lin <jilin@nvidia.com> - 2017-01-17 10:30 +0100

#1560322 — [PATCH] usb: gadget: configfs: Fix KASAN use-after-free

When gadget is disconnected, running sequence is like this.
. android_work: sent uevent USB_STATE=DISCONNECTED
. Call trace:
  usb_string_copy+0xd0/0x128
  gadget_config_name_configuration_store+0x4
  gadget_config_name_attr_store+0x40/0x50
  configfs_write_file+0x198/0x1f4
  vfs_write+0x100/0x220
  SyS_write+0x58/0xa8
. configfs_composite_unbind
. configfs_composite_bind

In configfs_composite_bind, it has
"cn->strings.s = cn->configuration;"

When usb_string_copy is invoked. it would
allocate memory, copy input string, release previous pointed memory space,
and use new allocated memory.

When gadget is connected, host sends down request to get information.
Call trace:
  usb_gadget_get_string+0xec/0x168
  lookup_string+0x64/0x98
  composite_setup+0xa34/0x1ee8
  android_setup+0xb4/0x140

If gadget is disconnected and connected quickly, in the failed case,
cn->configuration memory has been released by usb_string_copy kfree but
configfs_composite_bind hasn't been run in time to assign new allocated
"cn->configuration" pointer to "cn->strings.s".

When "strlen(s->s) of usb_gadget_get_string is being executed, the dangling
memory is accessed, "BUG: KASAN: use-after-free" error occurs.

Signed-off-by: Jim Lin <jilin@nvidia.com>
---
 drivers/usb/gadget/configfs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 78c4497..39fea62 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -106,6 +106,9 @@ struct gadget_config_name {
 	struct list_head list;
 };
 
+#define MAX_USB_STRING_LEN	126
+#define MAX_USB_STRING_WITH_NULL_LEN	(MAX_USB_STRING_LEN+1)
+
 static int usb_string_copy(const char *s, char **s_copy)
 {
 	int ret;
@@ -115,12 +118,16 @@ static int usb_string_copy(const char *s, char **s_copy)
 	if (ret > 126)
 		return -EOVERFLOW;
 
-	str = kstrdup(s, GFP_KERNEL);
-	if (!str)
-		return -ENOMEM;
+	if (copy) {
+		str = copy;
+	} else {
+		str = kmalloc(MAX_USB_STRING_WITH_NULL_LEN, GFP_KERNEL);
+		if (!str)
+			return -ENOMEM;
+	}
+	strcpy(str, s);
 	if (str[ret - 1] == '\n')
 		str[ret - 1] = '\0';
-	kfree(copy);
 	*s_copy = str;
 	return 0;
 }
-- 
2.7.4

[toc] | [next] | [standalone]

#1560329

Hi,

Jim Lin <jilin@nvidia.com> writes:
> When gadget is disconnected, running sequence is like this.
> . android_work: sent uevent USB_STATE=DISCONNECTED

I'm gonna have to ask you to try with actual mainline where there are no
Android changes.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1560383

On Tue, Jan 17, 2017 at 10:07:40AM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Jim Lin <jilin@nvidia.com> writes:
> > When gadget is disconnected, running sequence is like this.
> > . android_work: sent uevent USB_STATE=DISCONNECTED
> 
> I'm gonna have to ask you to try with actual mainline where there are no
> Android changes.

What is android changing these days in the gadget stack that is not
already upstream?

[toc] | [prev] | [next] | [standalone]

#1560397

[Multipart message — attachments visible in raw view] — view raw

Hi,

Greg KH <greg@kroah.com> writes:
> On Tue, Jan 17, 2017 at 10:07:40AM +0200, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Jim Lin <jilin@nvidia.com> writes:
>> > When gadget is disconnected, running sequence is like this.
>> > . android_work: sent uevent USB_STATE=DISCONNECTED
>> 
>> I'm gonna have to ask you to try with actual mainline where there are no
>> Android changes.
>
> What is android changing these days in the gadget stack that is not
> already upstream?

quite a bit, actually. They have their own android_setup() and an
android_worker thread for notifications. These notifications actually
duplicate (poorly) what we already have for usb_gadget_set_state(). They
also completely ditch composite_setup() to reimplement it with their own
additions. There's also an android class added to
configfs. Android-specific uevents. Android-specific ->disconnect()
implementation, overwriting what we have on composite.c. I just took a
diff from v4.4.10 to current Android head which we're using for some
other project

 drivers/usb/gadget/Kconfig                   |   50 +++++
 drivers/usb/gadget/composite.c               |    6 +
 drivers/usb/gadget/configfs.c                |  264 +++++++++++++++++++++++-
 drivers/usb/gadget/function/Makefile         |    8 +
 drivers/usb/gadget/function/f_accessory.c    | 1335 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/function/f_audio_source.c | 1060 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/function/f_fs.c           |   11 +-
 drivers/usb/gadget/function/f_midi.c         |   66 ++++++
 drivers/usb/gadget/function/f_mtp.c          | 1533 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/function/f_mtp.h          |   18 ++
 drivers/usb/gadget/function/f_ptp.c          |   38 ++++
 drivers/usb/gadget/function/f_rndis.c        |   30 +++
 drivers/usb/gadget/function/rndis.c          |  112 ++++++++--
 drivers/usb/gadget/function/rndis.h          |    2 +
 drivers/usb/gadget/function/u_ether.c        |  305 ++++++++++++++++++++++------
 drivers/usb/gadget/function/u_ether.h        |    3 +
 drivers/usb/gadget/functions.c               |    2 +-
 17 files changed, 4757 insertions(+), 86 deletions(-)

rather extensive.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1560384

On 2017年01月17日 16:07, Felipe Balbi wrote:
> Hi,
>
> Jim Lin <jilin@nvidia.com> writes:
>> When gadget is disconnected, running sequence is like this.
>> . android_work: sent uevent USB_STATE=DISCONNECTED
> I'm gonna have to ask you to try with actual mainline where there are no
> Android changes.
>
Let me rephrase and resend.

Thanks,

--nvpublic

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web