| Message ID | 20230714074011.20989-1-qiang.zhang1211@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | USB: gadget: Fix the memory leak in raw_gadget driver | expand |
On Fri, Jul 14, 2023 at 9:40 AM Zqiang <qiang.zhang1211@gmail.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@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=feb045d335c1fdde5bf7 > Signed-off-by: Zqiang <qiang.zhang1211@gmail.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@gmail.com> Tested-by: Andrey Konovalov <andreyknvl@gmail.com> Thanks!
> > On Fri, Jul 14, 2023 at 9:40 AM Zqiang <qiang.zhang1211@gmail.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@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=feb045d335c1fdde5bf7 > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.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@gmail.com> > Tested-by: Andrey Konovalov <andreyknvl@gmail.com> Thanks for review and test ! > > Thanks!
> > On Fri, Jul 14, 2023 at 9:40 AM Zqiang <qiang.zhang1211@gmail.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@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=feb045d335c1fdde5bf7 > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.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@gmail.com> > Tested-by: Andrey Konovalov <andreyknvl@gmail.com> > Hi Greg Friendly ping :) Thanks Zqiang > Thanks!
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; }
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@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=feb045d335c1fdde5bf7 Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> --- drivers/usb/gadget/legacy/raw_gadget.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)