Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CAEAjamvSRv3m1XZjS7pe2HptX20w7otn6hNU4YFvt7XF4WPPng@mail.gmail.com>
In-Reply-To: <CAEAjamvSRv3m1XZjS7pe2HptX20w7otn6hNU4YFvt7XF4WPPng@mail.gmail.com>
From:   Kyungtae Kim <kt0755@gmail.com>
Date:   Sat, 31 Oct 2020 22:16:05 -0400
Message-ID: <CAEAjamuZLCHS4geFiA5j7aUrw5p0bpefLQHZeYq6CA_LwqM7Bg@mail.gmail.com>
Subject: Re: [PATCH] USB: gadget: hidg: fix use-after-free in f_hidg
To:     Felipe Balbi <balbi@kernel.org>,
        Greg KH <gregkh@linuxfoundation.org>
Cc:     USB list <linux-usb@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        syzkaller <syzkaller@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Oct 28, 2020 at 4:13 PM Kyungtae Kim <kt0755@gmail.com> wrote:
>
> FuzzUSB (a variant of syzkaller) found the bug
> when accessing a freed instance of struct f_hidg.
>
> Reference: https://www.spinics.net/lists/linux-usb/msg195103.html
>
> The fix uses reference count to ensure the right access to instance of f_hidg.
>
>
> BUG: KASAN: use-after-free in f_hidg_poll+0x190/0x1e0 drivers/usb/gadget/function/f_hid.c:424
> Read of size 1 at addr ffff8880579260e8 by task syz-executor.5/2849
>
> CPU: 3 PID: 2849 Comm: syz-executor.5 Not tainted 5.6.11 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0xce/0x128 lib/dump_stack.c:118
>  print_address_description.constprop.4+0x21/0x3c0 mm/kasan/report.c:374
>  __kasan_report+0x131/0x1b0 mm/kasan/report.c:506
>  kasan_report+0x12/0x20 mm/kasan/common.c:641
>  __asan_report_load1_noabort+0x14/0x20 mm/kasan/generic_report.c:132
>  f_hidg_poll+0x190/0x1e0 drivers/usb/gadget/function/f_hid.c:424
>  vfs_poll include/linux/poll.h:90 [inline]
>  do_pollfd fs/select.c:859 [inline]
>  do_poll fs/select.c:907 [inline]
>  do_sys_poll+0x548/0xe20 fs/select.c:1001
>  __do_sys_poll fs/select.c:1059 [inline]
>  __se_sys_poll fs/select.c:1047 [inline]
>  __x64_sys_poll+0x171/0x420 fs/select.c:1047
>  do_syscall_64+0x9e/0x510 arch/x86/entry/common.c:294
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x4531a9
> Code: ed 60 fc ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 bb 60 fc ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007f07bfcd1c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000007
> RAX: ffffffffffffffda RBX: 000000000073bfa8 RCX: 00000000004531a9
> RDX: 0000000000000080 RSI: 0000000000000001 RDI: 0000000020001980
> RBP: 0000000000000003 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004bd290
> R13: 00000000004d2c28 R14: 00007f07bfcd26d4 R15: 00000000ffffffff
>
> Allocated by task 2418:
>  save_stack+0x21/0x90 mm/kasan/common.c:72
>  set_track mm/kasan/common.c:80 [inline]
>  __kasan_kmalloc.constprop.3+0xa7/0xd0 mm/kasan/common.c:515
>  kasan_kmalloc+0x9/0x10 mm/kasan/common.c:529
>  kmem_cache_alloc_trace+0xfa/0x2d0 mm/slub.c:2813
>  kzalloc include/linux/slab.h:555 [inline]
>  hidg_alloc+0x56/0x5e0 drivers/usb/gadget/function/f_hid.c:1091
>  usb_get_function+0x58/0xc0 drivers/usb/gadget/functions.c:61
>  config_usb_cfg_link+0x1ed/0x3e0 drivers/usb/gadget/configfs.c:444
>  configfs_symlink+0x527/0x11d0 fs/configfs/symlink.c:202
>  vfs_symlink+0x33d/0x5b0 fs/namei.c:4201
>  do_symlinkat+0x11b/0x1d0 fs/namei.c:4228
>  __do_sys_symlinkat fs/namei.c:4242 [inline]
>  __se_sys_symlinkat fs/namei.c:4239 [inline]
>  __x64_sys_symlinkat+0x73/0xb0 fs/namei.c:4239
>  do_syscall_64+0x9e/0x510 arch/x86/entry/common.c:294
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Freed by task 2868:
>  save_stack+0x21/0x90 mm/kasan/common.c:72
>  set_track mm/kasan/common.c:80 [inline]
>  kasan_set_free_info mm/kasan/common.c:337 [inline]
>  __kasan_slab_free+0x135/0x190 mm/kasan/common.c:476
>  kasan_slab_free+0xe/0x10 mm/kasan/common.c:485
>  slab_free_hook mm/slub.c:1444 [inline]
>  slab_free_freelist_hook mm/slub.c:1477 [inline]
>  slab_free mm/slub.c:3034 [inline]
>  kfree+0xf7/0x410 mm/slub.c:3995
>  hidg_free+0x7f/0x110 drivers/usb/gadget/function/f_hid.c:1069
>  usb_put_function+0x38/0x50 drivers/usb/gadget/functions.c:87
>  config_usb_cfg_unlink+0x2db/0x3b0 drivers/usb/gadget/configfs.c:485
>  configfs_unlink+0x3b9/0x7f0 fs/configfs/symlink.c:250
>  vfs_unlink+0x287/0x570 fs/namei.c:4073
>  do_unlinkat+0x4f9/0x620 fs/namei.c:4137
>  __do_sys_unlink fs/namei.c:4184 [inline]
>  __se_sys_unlink fs/namei.c:4182 [inline]
>  __x64_sys_unlink+0x42/0x50 fs/namei.c:4182
>  do_syscall_64+0x9e/0x510 arch/x86/entry/common.c:294
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
>
> Signed-off-by: Kyungtae Kim <kt0755@gmail.com>
> Reported-and-tested-by: Kyungtae Kim <kt0755@gmail.com>
>
> ---
>  drivers/usb/gadget/function/f_hid.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index 1125f4715830..e900b51c075a 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -16,6 +16,7 @@
>  #include <linux/wait.h>
>  #include <linux/sched.h>
>  #include <linux/usb/g_hid.h>
> +#include <linux/kref.h>
>
>  #include "u_f.h"
>  #include "u_hid.h"
> @@ -44,6 +45,7 @@ struct f_hidg {
>         unsigned short                  report_desc_length;
>         char                            *report_desc;
>         unsigned short                  report_length;
> +       struct kref             kref;
>
>         /* recv report */
>         struct list_head                completed_out_req;
> @@ -427,9 +429,21 @@ static __poll_t f_hidg_poll(struct file *file, poll_table *wait)
>  #undef WRITE_COND
>  #undef READ_COND
>
> +static void f_hidg_free(struct kref *kref)
> +{
> +       struct f_hidg *hidg = container_of(kref, struct f_hidg, kref);
> +
> +       kfree(hidg);
> +}
> +
>  static int f_hidg_release(struct inode *inode, struct file *fd)
>  {
> +       struct f_hidg   *hidg  = fd->private_data;
> +
>         fd->private_data = NULL;
> +
> +       kref_put(&hidg->kref, f_hidg_free);
> +
>         return 0;
>  }
>
> @@ -440,6 +454,8 @@ static int f_hidg_open(struct inode *inode, struct file *fd)
>
>         fd->private_data = hidg;
>
> +       kref_get(&hidg->kref);
> +
>         return 0;
>  }
>
> @@ -1060,7 +1076,9 @@ static void hidg_free(struct usb_function *f)
>         hidg = func_to_hidg(f);
>         opts = container_of(f->fi, struct f_hid_opts, func_inst);
>         kfree(hidg->report_desc);
> -       kfree(hidg);
> +
> +       kref_put(&hidg->kref, f_hidg_free);
> +
>         mutex_lock(&opts->lock);
>         --opts->refcnt;
>         mutex_unlock(&opts->lock);
> @@ -1089,6 +1107,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
>         opts = container_of(fi, struct f_hid_opts, func_inst);
>
>         mutex_lock(&opts->lock);
> +       kref_init(&hidg->kref);
>         ++opts->refcnt;
>
>         hidg->minor = opts->minor;
> --
> 2.17.1
>