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

  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@gmail.com>
Fixes: ac5688637144 ("media: em28xx: Fix possible memory leak of em28xx struct")
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
v1->v2: move kref_get to the original position
 drivers/media/usb/em28xx/em28xx-input.c | 1 -
 1 file changed, 1 deletion(-)
  

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@gmail.com>
> Fixes: ac5688637144 ("media: em28xx: Fix possible memory leak of em28xx struct")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
> 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
  

On Tue, Jul 13, 2021 at 4:48 PM Sean Young <sean@mess.org> 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@gmail.com>
> > Fixes: ac5688637144 ("media: em28xx: Fix possible memory leak of em28xx struct")
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> > 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
  

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.

[1] https://syzkaller.appspot.com/bug?id=a09553fd4df4c4a3824dc37a4040bf80d2600a50


On Tue, Jul 13, 2021 at 5:49 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> On Tue, Jul 13, 2021 at 4:48 PM Sean Young <sean@mess.org> 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@gmail.com>
> > > Fixes: ac5688637144 ("media: em28xx: Fix possible memory leak of em28xx struct")
> > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > ---
> > > 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
  

On Mon, Jul 19, 2021 at 5:57 PM Dongliang Mu <mudongliangabcd@gmail.com> 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@gmail.com> wrote:
> >
> > On Tue, Jul 13, 2021 at 4:48 PM Sean Young <sean@mess.org> 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@gmail.com>
> > > > Fixes: ac5688637144 ("media: em28xx: Fix possible memory leak of em28xx struct")
> > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > > ---
> > > > 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