On Mon, Jul 19, 2021 at 5:57 PM Dongliang Mu <mudongliangabcd@xxxxxxxxx> wrote: > > This can also fix another crash report - KASAN: use-after-free Read in > em28xx_close_extension [1]. It should be duplicated bug reports with > the memory leak. not memory leak, just the refcount bug mentioned in the commit message. > > [1] https://syzkaller.appspot.com/bug?id=a09553fd4df4c4a3824dc37a4040bf80d2600a50 > > > On Tue, Jul 13, 2021 at 5:49 PM Dongliang Mu <mudongliangabcd@xxxxxxxxx> wrote: > > > > On Tue, Jul 13, 2021 at 4:48 PM Sean Young <sean@xxxxxxxx> wrote: > > > > > > On Wed, Jul 07, 2021 at 05:34:09PM +0800, Dongliang Mu wrote: > > > > If em28xx_ir_init fails, it would decrease the refcount of dev. However, > > > > in the em28xx_ir_fini, when ir is NULL, it goes to ref_put and decrease > > > > the refcount of dev. This will lead to a refcount bug. > > > > > > > > Fix this bug by removing the kref_put in the error handling code > > > > of em28xx_ir_init. > > > > > > > > refcount_t: underflow; use-after-free. > > > > WARNING: CPU: 0 PID: 7 at lib/refcount.c:28 refcount_warn_saturate+0x18e/0x1a0 lib/refcount.c:28 > > > > Modules linked in: > > > > CPU: 0 PID: 7 Comm: kworker/0:1 Not tainted 5.13.0 #3 > > > > Workqueue: usb_hub_wq hub_event > > > > RIP: 0010:refcount_warn_saturate+0x18e/0x1a0 lib/refcount.c:28 > > > > Call Trace: > > > > kref_put.constprop.0+0x60/0x85 include/linux/kref.h:69 > > > > em28xx_usb_disconnect.cold+0xd7/0xdc drivers/media/usb/em28xx/em28xx-cards.c:4150 > > > > usb_unbind_interface+0xbf/0x3a0 drivers/usb/core/driver.c:458 > > > > __device_release_driver drivers/base/dd.c:1201 [inline] > > > > device_release_driver_internal+0x22a/0x230 drivers/base/dd.c:1232 > > > > bus_remove_device+0x108/0x160 drivers/base/bus.c:529 > > > > device_del+0x1fe/0x510 drivers/base/core.c:3540 > > > > usb_disable_device+0xd1/0x1d0 drivers/usb/core/message.c:1419 > > > > usb_disconnect+0x109/0x330 drivers/usb/core/hub.c:2221 > > > > hub_port_connect drivers/usb/core/hub.c:5151 [inline] > > > > hub_port_connect_change drivers/usb/core/hub.c:5440 [inline] > > > > port_event drivers/usb/core/hub.c:5586 [inline] > > > > hub_event+0xf81/0x1d40 drivers/usb/core/hub.c:5668 > > > > process_one_work+0x2c9/0x610 kernel/workqueue.c:2276 > > > > process_scheduled_works kernel/workqueue.c:2338 [inline] > > > > worker_thread+0x333/0x5b0 kernel/workqueue.c:2424 > > > > kthread+0x188/0x1d0 kernel/kthread.c:319 > > > > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295 > > > > > > > > Reported-by: Dongliang Mu <mudongliangabcd@xxxxxxxxx> > > > > Fixes: ac5688637144 ("media: em28xx: Fix possible memory leak of em28xx struct") > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@xxxxxxxxx> > > > > --- > > > > v1->v2: move kref_get to the original position > > > > drivers/media/usb/em28xx/em28xx-input.c | 1 - > > > > 1 file changed, 1 deletion(-) > > > > > > > > diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c > > > > index 59529cbf9cd0..0b6d77c3bec8 100644 > > > > --- a/drivers/media/usb/em28xx/em28xx-input.c > > > > +++ b/drivers/media/usb/em28xx/em28xx-input.c > > > > @@ -842,7 +842,6 @@ static int em28xx_ir_init(struct em28xx *dev) > > > > kfree(ir); > > > > ref_put: > > > > em28xx_shutdown_buttons(dev); > > > > - kref_put(&dev->ref, em28xx_free_device); > > > > return err; > > > > } > > > > > > Ideally we want an init function to not have any side effects if it returns > > > an error (or to do undo those side effects). With this change, the as long > > > as is_audio_only is not set, we always do a kref_get(), even in the error > > > case. As long as is_audio_only is not set, fini always does a kref_put(). > > > > > > Now this works but it's not really very readable code, and it requires that > > > the fini is called even if init returns an error. > > > > > > If an init function returns an error, it should undo any side effects like > > > allocations or reference counts. So the best way to handle this to only > > > do a kref_get() in the happy path of em28xx_ir_init(). > > > > Hi Sean, > > > > In the v1 version, I moved kref_get from the beginning to the ending. > > Do you mean this change? > > > > @@ -708,7 +708,6 @@ static int em28xx_ir_init(struct em28xx *dev) > > return 0; > > } > > > > - kref_get(&dev->ref); > > INIT_DELAYED_WORK(&dev->buttons_query_work, em28xx_query_buttons); > > > > if (dev->board.buttons) > > @@ -833,6 +832,9 @@ static int em28xx_ir_init(struct em28xx *dev) > > > > dev_info(&dev->intf->dev, "Input extension successfully initialized\n"); > > > > + /* Only increase refcount when this function is executed successfully */ > > + kref_get(&dev->ref); > > + > > return 0; > > > > Another reason not to move kref_get is that Paval comments on my patch: > > > > > kref_get() should be before this call to not trigger UAF in em28xx_query_buttons() > > > > So I think it's safe to call kref_get at the beginning. > > > > > > > > > > > Thanks > > > > > > Sean
