linux-kernel - Re: [PATCH] USB: gadget: Fix the memory leak in raw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 14 Jul 2023 23:43:21 +0200
From:   Andrey Konovalov <andreyknvl@...il.com>
To:     Zqiang <qiang.zhang1211@...il.com>, gregkh@...uxfoundation.org
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] USB: gadget: Fix the memory leak in raw_gadget driver

On Fri, Jul 14, 2023 at 9:40 AM Zqiang <qiang.zhang1211@...il.com> wrote:
>
> Currently, increasing raw_dev->count happens before invoke the
> raw_queue_event(), if the raw_queue_event() return error, invoke
> raw_release() will not trigger the dev_free() to be called.
>
> [  268.905865][ T5067] raw-gadget.0 gadget.0: failed to queue event
> [  268.912053][ T5067] udc dummy_udc.0: failed to start USB Raw Gadget: -12
> [  268.918885][ T5067] raw-gadget.0: probe of gadget.0 failed with error -12
> [  268.925956][ T5067] UDC core: USB Raw Gadget: couldn't find an available UDC or it's busy
> [  268.934657][ T5067] misc raw-gadget: fail, usb_gadget_register_driver returned -16
>
> BUG: memory leak
>
> [<ffffffff8154bf94>] kmalloc_trace+0x24/0x90 mm/slab_common.c:1076
> [<ffffffff8347eb55>] kmalloc include/linux/slab.h:582 [inline]
> [<ffffffff8347eb55>] kzalloc include/linux/slab.h:703 [inline]
> [<ffffffff8347eb55>] dev_new drivers/usb/gadget/legacy/raw_gadget.c:191 [inline]
> [<ffffffff8347eb55>] raw_open+0x45/0x110 drivers/usb/gadget/legacy/raw_gadget.c:385
> [<ffffffff827d1d09>] misc_open+0x1a9/0x1f0 drivers/char/misc.c:165
>
> [<ffffffff8154bf94>] kmalloc_trace+0x24/0x90 mm/slab_common.c:1076
> [<ffffffff8347cd2f>] kmalloc include/linux/slab.h:582 [inline]
> [<ffffffff8347cd2f>] raw_ioctl_init+0xdf/0x410 drivers/usb/gadget/legacy/raw_gadget.c:460
> [<ffffffff8347dfe9>] raw_ioctl+0x5f9/0x1120 drivers/usb/gadget/legacy/raw_gadget.c:1250
> [<ffffffff81685173>] vfs_ioctl fs/ioctl.c:51 [inline]
>
> [<ffffffff8154bf94>] kmalloc_trace+0x24/0x90 mm/slab_common.c:1076
> [<ffffffff833ecc6a>] kmalloc include/linux/slab.h:582 [inline]
> [<ffffffff833ecc6a>] kzalloc include/linux/slab.h:703 [inline]
> [<ffffffff833ecc6a>] dummy_alloc_request+0x5a/0xe0 drivers/usb/gadget/udc/dummy_hcd.c:665
> [<ffffffff833e9132>] usb_ep_alloc_request+0x22/0xd0 drivers/usb/gadget/udc/core.c:196
> [<ffffffff8347f13d>] gadget_bind+0x6d/0x370 drivers/usb/gadget/legacy/raw_gadget.c:292
>
> This commit therefore invoke kref_get() under the condition that
> raw_queue_event() return success.
>
> Reported-by: syzbot+feb045d335c1fdde5bf7@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=feb045d335c1fdde5bf7
> Signed-off-by: Zqiang <qiang.zhang1211@...il.com>
> ---
>  drivers/usb/gadget/legacy/raw_gadget.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> index 2acece16b890..e549022642e5 100644
> --- a/drivers/usb/gadget/legacy/raw_gadget.c
> +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> @@ -310,13 +310,15 @@ static int gadget_bind(struct usb_gadget *gadget,
>         dev->eps_num = i;
>         spin_unlock_irqrestore(&dev->lock, flags);
>
> -       /* Matches kref_put() in gadget_unbind(). */
> -       kref_get(&dev->count);
> -
>         ret = raw_queue_event(dev, USB_RAW_EVENT_CONNECT, 0, NULL);
> -       if (ret < 0)
> +       if (ret < 0) {
>                 dev_err(&gadget->dev, "failed to queue event\n");
> +               set_gadget_data(gadget, NULL);
> +               return ret;
> +       }
>
> +       /* Matches kref_put() in gadget_unbind(). */
> +       kref_get(&dev->count);
>         return ret;
>  }
>
> --
> 2.17.1

Indeed, if gadget_bind fails due to a raw_queue_event failure, the
core gadget code will never call gadget_unbind.

Reviewed-by: Andrey Konovalov <andreyknvl@...il.com>
Tested-by: Andrey Konovalov <andreyknvl@...il.com>

Thanks!