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

[Thadeu Lima de Souza Cascardo]

On Tue, Jan 17, 2017 at 12:29:09PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Jim Lin <jilin@xxxxxxxxxx> writes:
> > When gadget is disconnected, running sequence is like this.
> > . composite_disconnect
> > . 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
> >
> > 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@xxxxxxxxxx>
> > ---
> > Changes in v2:
> > Changes in v3:
> >  Change commit description
> 
> well, I need to be sure you tested this with Linus' tree. The reason I'm
> asking is because this could be a bug caused by Android changes. From
> your previous patch, the problem started with android_setup().
> 
> Please test with v4.10-rc4 and any configfs-based gadget.
> 
> -- 
> balbi

I tested this with dummy_hcd on top of a 5.8 kernel and I got lsusb to respond
with an error instead of the right manufacturer string, after overwriting such
a string after binding.

With the patch applied, after the string is overwritten, lsusb will show the
updated string.

Because of commit 81c7462883b0cc0a4eeef0687f80ad5b5baee5f6 ("USB: replace
hardcode maximum usb string length by definition"), the patch will need a
fixup. Should I send a v2 with my sign-off?

Thanks.
Cascardo.