Re: [PATCH v2] [media] em28xx-input: fix refcount bug in em28xx_usb_disconnect

[Sean Young]

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().


Thanks

Sean