Re: [PATCH 1/1] usb: gadget: f_hid: Conduct proper refcounting on shared f_hidg pointer

From: John Keeping
Date: Sun Nov 20 2022 - 12:22:43 EST


On Fri, Nov 18, 2022 at 04:07:24PM -0500, Alan Stern wrote:
> On Fri, Nov 18, 2022 at 04:37:32PM +0000, John Keeping wrote:
> > I don't think it's at all simple to fix this - I posted a series
> > addressing the lifetime issues here a few years ago but didn't chase it
> > up and there was no feedback:
> >
> > https://lore.kernel.org/linux-usb/20191028114228.3679219-1-john@xxxxxxxxxxxx/
> >
> > That includes a patch to remove the embedded struct cdev and manage its
> > lifetime separately, which I think is needed as there are two different
> > struct device objects here and we cannot tie their lifetimes together.
>
> I still don't have a clear picture of what the real problem is. Lee's
> original patch description just said "external references are presently
> not tracked", with no details about what those external references are.
> Why not add just proper cdev_get() and cdev_put() calls to whatever code
> handles those external references, so that they _are_ tracked?
>
> What are the two different struct device objects? Why do their
> lifetimes need to be tied together? If you do need to tie their
> lifetimes somehow, why not simply make one of them (the one which is
> logically allowed to be shorter-lived) hold a reference to the other?

The problem is that we have a struct cdev embedded in f_hidg but the
lifetime of f_hidg is not tied to any kobject so we can't solve this in
the right way by setting the parent kobject of the cdev.

While refcounting struct f_hidg is necessary, it's not sufficient
because the only way to keep it alive long enough for the final
kobject_put() on the embedded cdev is to tie the lifetime to a kobject
of its own and there is no suitable object as this is not the model
followed by gadget function instances.

To show the problem (using libusbgx's example commands for conciseness,
but obviously this can be done via configfs directly):

$ gadget-hid
$ exec 3<> /dev/hidg0
$ gadget-vid-pid-remove
$ exec 3<&-

==================================================================
BUG: KASAN: use-after-free in kobject_put+0x24/0x250
Read of size 1 at addr c61784a0 by task sh/264

CPU: 1 PID: 264 Comm: sh Tainted: G W 6.0.5 #1
Hardware name: Rockchip (Device Tree)
unwind_backtrace from show_stack+0x10/0x14
show_stack from dump_stack_lvl+0x58/0x70
dump_stack_lvl from print_report+0x58/0x4dc
print_report from kasan_report+0x88/0xa8
kasan_report from kobject_put+0x24/0x250
kobject_put from __fput+0x1e4/0x358
__fput from task_work_run+0xb4/0xc4
task_work_run from do_work_pending+0x4d4/0x524
do_work_pending from slow_work_pending+0xc/0x20
Exception stack(0xf284bfb0 to 0xf284bff8)
bfa0: 00000000 00000003 02292190 00000000
bfc0: 02293fc8 aed4e1d0 00000001 00000006 022925a0 00000000 022925a0 022923f4
bfe0: 00532f38 b6864840 0049c218 aec57e38 60000010 0000000b

Allocated by task 341:
kasan_set_track+0x20/0x28
____kasan_kmalloc+0x80/0x88
hidg_alloc+0x24/0x1f0
usb_get_function+0x28/0x48
config_usb_cfg_link+0x90/0x114
configfs_symlink+0x24c/0x5d8
vfs_symlink+0x58/0x74
do_symlinkat+0xdc/0x16c
ret_fast_syscall+0x0/0x1c

Freed by task 352:
kasan_set_track+0x20/0x28
kasan_set_free_info+0x28/0x34
____kasan_slab_free+0xf8/0x108
__kasan_slab_free+0x10/0x18
slab_free_freelist_hook+0x9c/0xfc
kfree+0x118/0x258
hidg_free+0x44/0x6c
config_usb_cfg_unlink+0xd4/0xf4
configfs_unlink+0x118/0x15c
vfs_unlink+0xd8/0x1c4
do_unlinkat+0x180/0x28c
ret_fast_syscall+0x0/0x1c

The buggy address belongs to the object at c6178400
which belongs to the cache kmalloc-512 of size 512
The buggy address is located 160 bytes inside of
512-byte region [c6178400, c6178600)