Re: [syzbot] memory leak in hub_event (3)

Re: [syzbot] memory leak in hub_event (3)

[Alan Stern]

On Fri, Feb 11, 2022 at 12:17:26PM -0800, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    dfd42facf1e4 Linux 5.17-rc3
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=14b4ef7c700000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=48b71604a367da6e
> dashboard link: https://syzkaller.appspot.com/bug?extid=8caaaec4e7a55d75e243
> compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1396902c700000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1466e662700000
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+8caaaec4e7a55d75e243@syzkaller.appspotmail.com
> 
> BUG: memory leak
> unreferenced object 0xffff88810d49e800 (size 2048):
>   comm "kworker/1:1", pid 25, jiffies 4294954629 (age 16.460s)
>   hex dump (first 32 bytes):
>     ff ff ff ff 31 00 00 00 00 00 00 00 00 00 00 00  ....1...........
>     00 00 00 00 00 00 00 00 00 00 00 00 03 00 00 00  ................
>   backtrace:
>     [<ffffffff82c87a62>] kmalloc include/linux/slab.h:581 [inline]
>     [<ffffffff82c87a62>] kzalloc include/linux/slab.h:715 [inline]
>     [<ffffffff82c87a62>] usb_alloc_dev+0x32/0x450 drivers/usb/core/usb.c:582
>     [<ffffffff82c91a47>] hub_port_connect drivers/usb/core/hub.c:5260 [inline]
>     [<ffffffff82c91a47>] hub_port_connect_change drivers/usb/core/hub.c:5502 [inline]
>     [<ffffffff82c91a47>] port_event drivers/usb/core/hub.c:5660 [inline]
>     [<ffffffff82c91a47>] hub_event+0x1097/0x21a0 drivers/usb/core/hub.c:5742
>     [<ffffffff8126c3ef>] process_one_work+0x2bf/0x600 kernel/workqueue.c:2307
>     [<ffffffff8126ccd9>] worker_thread+0x59/0x5b0 kernel/workqueue.c:2454
>     [<ffffffff81276765>] kthread+0x125/0x160 kernel/kthread.c:377
>     [<ffffffff810022ff>] ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295

There's a refcount leak in the probe-failure path of the hid-elo driver.  
(You can see that this is the relevant driver in the console output.)  
It doesn't need the refcount anyway, because the elo_priv structure is 
always deallocated synchronously before the elo_remove routine returns.

(Syzbot isn't always all that great at deducing where the real problem 
lies when something goes wrong.)

Alan Stern

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v5.17-rc3

Index: usb-devel/drivers/hid/hid-elo.c
===================================================================
--- usb-devel.orig/drivers/hid/hid-elo.c
+++ usb-devel/drivers/hid/hid-elo.c
@@ -239,7 +239,7 @@ static int elo_probe(struct hid_device *
 
 	INIT_DELAYED_WORK(&priv->work, elo_work);
 	udev = interface_to_usbdev(to_usb_interface(hdev->dev.parent));
-	priv->usbdev = usb_get_dev(udev);
+	priv->usbdev = udev;
 
 	hid_set_drvdata(hdev, priv);
 
@@ -270,8 +270,6 @@ static void elo_remove(struct hid_device
 {
 	struct elo_priv *priv = hid_get_drvdata(hdev);
 
-	usb_put_dev(priv->usbdev);
-
 	hid_hw_stop(hdev);
 	cancel_delayed_work_sync(&priv->work);
 	kfree(priv);

Re: [syzbot] memory leak in hub_event (3)

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+8caaaec4e7a55d75e243@syzkaller.appspotmail.com

Tested on:

commit:         dfd42fac Linux 5.17-rc3
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v5.17-rc3
kernel config:  https://syzkaller.appspot.com/x/.config?x=48b71604a367da6e
dashboard link: https://syzkaller.appspot.com/bug?extid=8caaaec4e7a55d75e243
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=121f0f78700000

Note: testing is done by a robot and is best-effort only.

[PATCH] HID: elo: Fix refcount leak in elo_probe()

[Alan Stern]

Syzbot identified a refcount leak in the hid-elo driver:

BUG: memory leak
unreferenced object 0xffff88810d49e800 (size 2048):
  comm "kworker/1:1", pid 25, jiffies 4294954629 (age 16.460s)
  hex dump (first 32 bytes):
    ff ff ff ff 31 00 00 00 00 00 00 00 00 00 00 00  ....1...........
    00 00 00 00 00 00 00 00 00 00 00 00 03 00 00 00  ................
  backtrace:
    [<ffffffff82c87a62>] kmalloc include/linux/slab.h:581 [inline]
    [<ffffffff82c87a62>] kzalloc include/linux/slab.h:715 [inline]
    [<ffffffff82c87a62>] usb_alloc_dev+0x32/0x450 drivers/usb/core/usb.c:582
    [<ffffffff82c91a47>] hub_port_connect drivers/usb/core/hub.c:5260 [inline]
    [<ffffffff82c91a47>] hub_port_connect_change drivers/usb/core/hub.c:5502 [inline]
    [<ffffffff82c91a47>] port_event drivers/usb/core/hub.c:5660 [inline]
    [<ffffffff82c91a47>] hub_event+0x1097/0x21a0 drivers/usb/core/hub.c:5742
    [<ffffffff8126c3ef>] process_one_work+0x2bf/0x600 kernel/workqueue.c:2307
    [<ffffffff8126ccd9>] worker_thread+0x59/0x5b0 kernel/workqueue.c:2454
    [<ffffffff81276765>] kthread+0x125/0x160 kernel/kthread.c:377
    [<ffffffff810022ff>] ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295

Not shown in the bug report but present in the console log:

[  182.014764][ T3257] elo 0003:04E7:0030.0006: item fetching failed at offset 0/1
[  182.022255][ T3257] elo 0003:04E7:0030.0006: parse failed
[  182.027904][ T3257] elo: probe of 0003:04E7:0030.0006 failed with error -22
[  182.214767][ T3257] usb 1-1: USB disconnect, device number 7
[  188.090199][ T3604] kmemleak: 3 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
BUG: memory leak

which points to hid-elo as the buggy driver.

The leak is caused by elo_probe() failing to release the reference it
holds to the struct usb_device in its failure pathway.  In the end the
driver doesn't need to take this reference at all, because the
elo_priv structure is always deallocated synchronously when the driver
unbinds from the interface.

Therefore this patch fixes the reference leak by not taking the
reference in the first place.

Reported-and-tested-by: syzbot+8caaaec4e7a55d75e243@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: <stable@vger.kernel.org>

---


[as1971]


 drivers/hid/hid-elo.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Index: usb-devel/drivers/hid/hid-elo.c
===================================================================
--- usb-devel.orig/drivers/hid/hid-elo.c
+++ usb-devel/drivers/hid/hid-elo.c
@@ -239,7 +239,7 @@ static int elo_probe(struct hid_device *
 
 	INIT_DELAYED_WORK(&priv->work, elo_work);
 	udev = interface_to_usbdev(to_usb_interface(hdev->dev.parent));
-	priv->usbdev = usb_get_dev(udev);
+	priv->usbdev = udev;
 
 	hid_set_drvdata(hdev, priv);
 
@@ -270,8 +270,6 @@ static void elo_remove(struct hid_device
 {
 	struct elo_priv *priv = hid_get_drvdata(hdev);
 
-	usb_put_dev(priv->usbdev);
-
 	hid_hw_stop(hdev);
 	cancel_delayed_work_sync(&priv->work);
 	kfree(priv);

Re: [PATCH] HID: elo: Fix refcount leak in elo_probe()

[Dongliang Mu]

On Sat, Feb 12, 2022 at 9:50 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> Syzbot identified a refcount leak in the hid-elo driver:
>
> BUG: memory leak
> unreferenced object 0xffff88810d49e800 (size 2048):
>   comm "kworker/1:1", pid 25, jiffies 4294954629 (age 16.460s)
>   hex dump (first 32 bytes):
>     ff ff ff ff 31 00 00 00 00 00 00 00 00 00 00 00  ....1...........
>     00 00 00 00 00 00 00 00 00 00 00 00 03 00 00 00  ................
>   backtrace:
>     [<ffffffff82c87a62>] kmalloc include/linux/slab.h:581 [inline]
>     [<ffffffff82c87a62>] kzalloc include/linux/slab.h:715 [inline]
>     [<ffffffff82c87a62>] usb_alloc_dev+0x32/0x450 drivers/usb/core/usb.c:582
>     [<ffffffff82c91a47>] hub_port_connect drivers/usb/core/hub.c:5260 [inline]
>     [<ffffffff82c91a47>] hub_port_connect_change drivers/usb/core/hub.c:5502 [inline]
>     [<ffffffff82c91a47>] port_event drivers/usb/core/hub.c:5660 [inline]
>     [<ffffffff82c91a47>] hub_event+0x1097/0x21a0 drivers/usb/core/hub.c:5742
>     [<ffffffff8126c3ef>] process_one_work+0x2bf/0x600 kernel/workqueue.c:2307
>     [<ffffffff8126ccd9>] worker_thread+0x59/0x5b0 kernel/workqueue.c:2454
>     [<ffffffff81276765>] kthread+0x125/0x160 kernel/kthread.c:377
>     [<ffffffff810022ff>] ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
>
> Not shown in the bug report but present in the console log:
>
> [  182.014764][ T3257] elo 0003:04E7:0030.0006: item fetching failed at offset 0/1
> [  182.022255][ T3257] elo 0003:04E7:0030.0006: parse failed
> [  182.027904][ T3257] elo: probe of 0003:04E7:0030.0006 failed with error -22
> [  182.214767][ T3257] usb 1-1: USB disconnect, device number 7
> [  188.090199][ T3604] kmemleak: 3 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
> BUG: memory leak
>
> which points to hid-elo as the buggy driver.
>
> The leak is caused by elo_probe() failing to release the reference it
> holds to the struct usb_device in its failure pathway.  In the end the
> driver doesn't need to take this reference at all, because the

Hi Alan,

My patch "[PATCH] hid: elo: fix memory leak in elo_probe" is merged
several weeks ago.

However, I fix this bug by modifying the error handling code in
elo_probe. If you think the refcount is not necessary, maybe a new
patch to remove the refcount is better.

> elo_priv structure is always deallocated synchronously when the driver
> unbinds from the interface.
>
> Therefore this patch fixes the reference leak by not taking the
> reference in the first place.
>
> Reported-and-tested-by: syzbot+8caaaec4e7a55d75e243@syzkaller.appspotmail.com
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> CC: <stable@vger.kernel.org>
>
> ---
>
>
> [as1971]
>
>
>  drivers/hid/hid-elo.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> Index: usb-devel/drivers/hid/hid-elo.c
> ===================================================================
> --- usb-devel.orig/drivers/hid/hid-elo.c
> +++ usb-devel/drivers/hid/hid-elo.c
> @@ -239,7 +239,7 @@ static int elo_probe(struct hid_device *
>
>         INIT_DELAYED_WORK(&priv->work, elo_work);
>         udev = interface_to_usbdev(to_usb_interface(hdev->dev.parent));
> -       priv->usbdev = usb_get_dev(udev);
> +       priv->usbdev = udev;
>
>         hid_set_drvdata(hdev, priv);
>
> @@ -270,8 +270,6 @@ static void elo_remove(struct hid_device
>  {
>         struct elo_priv *priv = hid_get_drvdata(hdev);
>
> -       usb_put_dev(priv->usbdev);
> -
>         hid_hw_stop(hdev);
>         cancel_delayed_work_sync(&priv->work);
>         kfree(priv);
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/YgcSbUwiALbmoTvL%40rowland.harvard.edu.

Re: [PATCH] HID: elo: Fix refcount leak in elo_probe()

[Alan Stern]

On Mon, Feb 14, 2022 at 03:34:42PM +0800, Dongliang Mu wrote:
> On Sat, Feb 12, 2022 at 9:50 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > Syzbot identified a refcount leak in the hid-elo driver:
> >
> > BUG: memory leak
> > unreferenced object 0xffff88810d49e800 (size 2048):
> >   comm "kworker/1:1", pid 25, jiffies 4294954629 (age 16.460s)
> >   hex dump (first 32 bytes):
> >     ff ff ff ff 31 00 00 00 00 00 00 00 00 00 00 00  ....1...........
> >     00 00 00 00 00 00 00 00 00 00 00 00 03 00 00 00  ................
> >   backtrace:
> >     [<ffffffff82c87a62>] kmalloc include/linux/slab.h:581 [inline]
> >     [<ffffffff82c87a62>] kzalloc include/linux/slab.h:715 [inline]
> >     [<ffffffff82c87a62>] usb_alloc_dev+0x32/0x450 drivers/usb/core/usb.c:582
> >     [<ffffffff82c91a47>] hub_port_connect drivers/usb/core/hub.c:5260 [inline]
> >     [<ffffffff82c91a47>] hub_port_connect_change drivers/usb/core/hub.c:5502 [inline]
> >     [<ffffffff82c91a47>] port_event drivers/usb/core/hub.c:5660 [inline]
> >     [<ffffffff82c91a47>] hub_event+0x1097/0x21a0 drivers/usb/core/hub.c:5742
> >     [<ffffffff8126c3ef>] process_one_work+0x2bf/0x600 kernel/workqueue.c:2307
> >     [<ffffffff8126ccd9>] worker_thread+0x59/0x5b0 kernel/workqueue.c:2454
> >     [<ffffffff81276765>] kthread+0x125/0x160 kernel/kthread.c:377
> >     [<ffffffff810022ff>] ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
> >
> > Not shown in the bug report but present in the console log:
> >
> > [  182.014764][ T3257] elo 0003:04E7:0030.0006: item fetching failed at offset 0/1
> > [  182.022255][ T3257] elo 0003:04E7:0030.0006: parse failed
> > [  182.027904][ T3257] elo: probe of 0003:04E7:0030.0006 failed with error -22
> > [  182.214767][ T3257] usb 1-1: USB disconnect, device number 7
> > [  188.090199][ T3604] kmemleak: 3 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
> > BUG: memory leak
> >
> > which points to hid-elo as the buggy driver.
> >
> > The leak is caused by elo_probe() failing to release the reference it
> > holds to the struct usb_device in its failure pathway.  In the end the
> > driver doesn't need to take this reference at all, because the
> 
> Hi Alan,
> 
> My patch "[PATCH] hid: elo: fix memory leak in elo_probe" is merged
> several weeks ago.

Really?  It still isn't in Linus's tree as of 5.17-rc4.  I would expect 
a bug fix to go upstream as soon as possible.

> However, I fix this bug by modifying the error handling code in
> elo_probe. If you think the refcount is not necessary, maybe a new
> patch to remove the refcount is better.

The refcount was added less than a year ago by Salah Triki in commit 
fbf42729d0e9 ("HID: elo: update the reference count of the usb device 
structure"), but the commit message doesn't explain why it is 
necessary.  There certainly isn't any obvious reason for it; the driver 
doesn't release any references after elo_remove() returns and we know 
that the usb_device structure won't be deallocated before the driver 
gets unbound.

Alan Stern

Re: [PATCH] HID: elo: Fix refcount leak in elo_probe()

[Dan Carpenter]

On Mon, Feb 14, 2022 at 09:41:32AM -0500, Alan Stern wrote:
> On Mon, Feb 14, 2022 at 03:34:42PM +0800, Dongliang Mu wrote:
> > On Sat, Feb 12, 2022 at 9:50 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > Syzbot identified a refcount leak in the hid-elo driver:
> > >
> > > BUG: memory leak
> > > unreferenced object 0xffff88810d49e800 (size 2048):
> > >   comm "kworker/1:1", pid 25, jiffies 4294954629 (age 16.460s)
> > >   hex dump (first 32 bytes):
> > >     ff ff ff ff 31 00 00 00 00 00 00 00 00 00 00 00  ....1...........
> > >     00 00 00 00 00 00 00 00 00 00 00 00 03 00 00 00  ................
> > >   backtrace:
> > >     [<ffffffff82c87a62>] kmalloc include/linux/slab.h:581 [inline]
> > >     [<ffffffff82c87a62>] kzalloc include/linux/slab.h:715 [inline]
> > >     [<ffffffff82c87a62>] usb_alloc_dev+0x32/0x450 drivers/usb/core/usb.c:582
> > >     [<ffffffff82c91a47>] hub_port_connect drivers/usb/core/hub.c:5260 [inline]
> > >     [<ffffffff82c91a47>] hub_port_connect_change drivers/usb/core/hub.c:5502 [inline]
> > >     [<ffffffff82c91a47>] port_event drivers/usb/core/hub.c:5660 [inline]
> > >     [<ffffffff82c91a47>] hub_event+0x1097/0x21a0 drivers/usb/core/hub.c:5742
> > >     [<ffffffff8126c3ef>] process_one_work+0x2bf/0x600 kernel/workqueue.c:2307
> > >     [<ffffffff8126ccd9>] worker_thread+0x59/0x5b0 kernel/workqueue.c:2454
> > >     [<ffffffff81276765>] kthread+0x125/0x160 kernel/kthread.c:377
> > >     [<ffffffff810022ff>] ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
> > >
> > > Not shown in the bug report but present in the console log:
> > >
> > > [  182.014764][ T3257] elo 0003:04E7:0030.0006: item fetching failed at offset 0/1
> > > [  182.022255][ T3257] elo 0003:04E7:0030.0006: parse failed
> > > [  182.027904][ T3257] elo: probe of 0003:04E7:0030.0006 failed with error -22
> > > [  182.214767][ T3257] usb 1-1: USB disconnect, device number 7
> > > [  188.090199][ T3604] kmemleak: 3 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
> > > BUG: memory leak
> > >
> > > which points to hid-elo as the buggy driver.
> > >
> > > The leak is caused by elo_probe() failing to release the reference it
> > > holds to the struct usb_device in its failure pathway.  In the end the
> > > driver doesn't need to take this reference at all, because the
> > 
> > Hi Alan,
> > 
> > My patch "[PATCH] hid: elo: fix memory leak in elo_probe" is merged
> > several weeks ago.
> 
> Really?  It still isn't in Linus's tree as of 5.17-rc4.  I would expect 
> a bug fix to go upstream as soon as possible.
> 
> > However, I fix this bug by modifying the error handling code in
> > elo_probe. If you think the refcount is not necessary, maybe a new
> > patch to remove the refcount is better.
> 
> The refcount was added less than a year ago by Salah Triki in commit 
> fbf42729d0e9 ("HID: elo: update the reference count of the usb device 
> structure"), but the commit message doesn't explain why it is 
> necessary.  There certainly isn't any obvious reason for it; the driver 
> doesn't release any references after elo_remove() returns and we know 
> that the usb_device structure won't be deallocated before the driver 
> gets unbound.

Salah sent a bunch of these.  The reasoning was explained in this email.

https://www.spinics.net/lists/kernel/msg4026672.html

When he resent the patch, Greg said that taking the reference wasn't
needed so the patch wasn't applied.  (Also it had the same reference
leak so that's a second reason it wasn't applied).

https://lkml.org/lkml/2021/8/4/324

So someone should go through and revert all of Salah's bogus usb_get_dev()
patches.

regards,
dan carpenter

Re: [PATCH] HID: elo: Fix refcount leak in elo_probe()

[Dan Carpenter]

On Thu, Feb 17, 2022 at 11:04:59AM +0300, Dan Carpenter wrote:
> 
> Salah sent a bunch of these.  The reasoning was explained in this email.
> 
> https://www.spinics.net/lists/kernel/msg4026672.html
> 
> When he resent the patch, Greg said that taking the reference wasn't
> needed so the patch wasn't applied.  (Also it had the same reference
> leak so that's a second reason it wasn't applied).
> 
> https://lkml.org/lkml/2021/8/4/324
> 
> So someone should go through and revert all of Salah's bogus usb_get_dev()
> patches.

Never mind, this is the only usb_get_dev() which was merged.

regards,
dan carpenter

Re: [PATCH] HID: elo: Fix refcount leak in elo_probe()

[Jiri Kosina]

On Thu, 17 Feb 2022, Dan Carpenter wrote:

> > The refcount was added less than a year ago by Salah Triki in commit 
> > fbf42729d0e9 ("HID: elo: update the reference count of the usb device 
> > structure"), but the commit message doesn't explain why it is 
> > necessary.  There certainly isn't any obvious reason for it; the driver 
> > doesn't release any references after elo_remove() returns and we know 
> > that the usb_device structure won't be deallocated before the driver 
> > gets unbound.
> 
> Salah sent a bunch of these.  The reasoning was explained in this email.
> 
> https://www.spinics.net/lists/kernel/msg4026672.html
> 
> When he resent the patch, Greg said that taking the reference wasn't
> needed so the patch wasn't applied.  (Also it had the same reference
> leak so that's a second reason it wasn't applied).

Sorry for late response, I've been away for a week. I have now queued 
revert of all this mess and will be sending it to Linus for 5.17 still. 
Thanks everybody for reporting.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: elo: Fix refcount leak in elo_probe()

[Alan Stern]

On Thu, Feb 17, 2022 at 11:04:59AM +0300, Dan Carpenter wrote:
> Salah sent a bunch of these.  The reasoning was explained in this email.
> 
> https://www.spinics.net/lists/kernel/msg4026672.html
> 
> When he resent the patch, Greg said that taking the reference wasn't
> needed so the patch wasn't applied.  (Also it had the same reference
> leak so that's a second reason it wasn't applied).

Indeed, the kerneldoc for usb_get_intf() does say that each reference 
held by a driver must be refcounted.  And there's nothing wrong with 
doing that, _provided_ you do it correctly.

But if you know the extra refcount will never be needed (because the 
reference will be dropped before the usb_interface in question is 
removed), fiddling with the reference count is unnecessary.  I guess 
whether or not to do it could be considered a matter of taste.

On the other hand, it wouldn't hurt to update the kerneldoc for 
usb_get_intf() (and usb_get_dev() also).  We could point out that if a 
driver does not access the usb_interface structure after its disconnect 
routine returns, incrementing the refcount isn't mandatory.

Greg, any opinion on this?

Alan Stern

Re: [PATCH] HID: elo: Fix refcount leak in elo_probe()

[Greg KH]

On Thu, Feb 17, 2022 at 10:25:02AM -0500, Alan Stern wrote:
> On Thu, Feb 17, 2022 at 11:04:59AM +0300, Dan Carpenter wrote:
> > Salah sent a bunch of these.  The reasoning was explained in this email.
> > 
> > https://www.spinics.net/lists/kernel/msg4026672.html
> > 
> > When he resent the patch, Greg said that taking the reference wasn't
> > needed so the patch wasn't applied.  (Also it had the same reference
> > leak so that's a second reason it wasn't applied).
> 
> Indeed, the kerneldoc for usb_get_intf() does say that each reference 
> held by a driver must be refcounted.  And there's nothing wrong with 
> doing that, _provided_ you do it correctly.
> 
> But if you know the extra refcount will never be needed (because the 
> reference will be dropped before the usb_interface in question is 
> removed), fiddling with the reference count is unnecessary.  I guess 
> whether or not to do it could be considered a matter of taste.
> 
> On the other hand, it wouldn't hurt to update the kerneldoc for 
> usb_get_intf() (and usb_get_dev() also).  We could point out that if a 
> driver does not access the usb_interface structure after its disconnect 
> routine returns, incrementing the refcount isn't mandatory.

That would be good to add to prevent this type of confusion in the
future.

thanks,

greg k-h

Re: [PATCH] HID: elo: Fix refcount leak in elo_probe()

[Dongliang Mu]

On Fri, Feb 25, 2022 at 5:15 PM Greg KH <greg@kroah.com> wrote:
>
> On Thu, Feb 17, 2022 at 10:25:02AM -0500, Alan Stern wrote:
> > On Thu, Feb 17, 2022 at 11:04:59AM +0300, Dan Carpenter wrote:
> > > Salah sent a bunch of these.  The reasoning was explained in this email.
> > >
> > > https://www.spinics.net/lists/kernel/msg4026672.html
> > >
> > > When he resent the patch, Greg said that taking the reference wasn't
> > > needed so the patch wasn't applied.  (Also it had the same reference
> > > leak so that's a second reason it wasn't applied).
> >
> > Indeed, the kerneldoc for usb_get_intf() does say that each reference
> > held by a driver must be refcounted.  And there's nothing wrong with
> > doing that, _provided_ you do it correctly.
> >
> > But if you know the extra refcount will never be needed (because the
> > reference will be dropped before the usb_interface in question is
> > removed), fiddling with the reference count is unnecessary.  I guess
> > whether or not to do it could be considered a matter of taste.
> >
> > On the other hand, it wouldn't hurt to update the kerneldoc for
> > usb_get_intf() (and usb_get_dev() also).  We could point out that if a
> > driver does not access the usb_interface structure after its disconnect
> > routine returns, incrementing the refcount isn't mandatory.
>
> That would be good to add to prevent this type of confusion in the
> future.

Hi Jiri Kosina,

from my understanding, my previous patch and patch from Alan Stern can
all fix the underlying issue. But as Dan said, the patch from Alan is
more elegant.

However, the revert commit from you said, my commit introduces memory
leak, which confuses me.

HID: elo: Revert USB reference counting

Commit 817b8b9 ("HID: elo: fix memory leak in elo_probe") introduced
memory leak on error path, but more importantly the whole USB reference
counting is not needed at all in the first place ......

If it is really my patch that introduces the memory leak, please let me know.

best regards,
Dongliang Mu

>
> thanks,
>
> greg k-h

Re: [PATCH] HID: elo: Fix refcount leak in elo_probe()

[Alan Stern]

On Sat, Mar 12, 2022 at 05:39:05PM +0800, Dongliang Mu wrote:
> On Fri, Feb 25, 2022 at 5:15 PM Greg KH <greg@kroah.com> wrote:
> >
> > On Thu, Feb 17, 2022 at 10:25:02AM -0500, Alan Stern wrote:
> > > On Thu, Feb 17, 2022 at 11:04:59AM +0300, Dan Carpenter wrote:
> > > > Salah sent a bunch of these.  The reasoning was explained in this email.
> > > >
> > > > https://www.spinics.net/lists/kernel/msg4026672.html
> > > >
> > > > When he resent the patch, Greg said that taking the reference wasn't
> > > > needed so the patch wasn't applied.  (Also it had the same reference
> > > > leak so that's a second reason it wasn't applied).
> > >
> > > Indeed, the kerneldoc for usb_get_intf() does say that each reference
> > > held by a driver must be refcounted.  And there's nothing wrong with
> > > doing that, _provided_ you do it correctly.
> > >
> > > But if you know the extra refcount will never be needed (because the
> > > reference will be dropped before the usb_interface in question is
> > > removed), fiddling with the reference count is unnecessary.  I guess
> > > whether or not to do it could be considered a matter of taste.
> > >
> > > On the other hand, it wouldn't hurt to update the kerneldoc for
> > > usb_get_intf() (and usb_get_dev() also).  We could point out that if a
> > > driver does not access the usb_interface structure after its disconnect
> > > routine returns, incrementing the refcount isn't mandatory.
> >
> > That would be good to add to prevent this type of confusion in the
> > future.
> 
> Hi Jiri Kosina,
> 
> from my understanding, my previous patch and patch from Alan Stern can
> all fix the underlying issue. But as Dan said, the patch from Alan is
> more elegant.
> 
> However, the revert commit from you said, my commit introduces memory
> leak, which confuses me.
> 
> HID: elo: Revert USB reference counting
> 
> Commit 817b8b9 ("HID: elo: fix memory leak in elo_probe") introduced
> memory leak on error path, but more importantly the whole USB reference
> counting is not needed at all in the first place ......
> 
> If it is really my patch that introduces the memory leak, please let me know.

Jiri named the wrong commit in his Changelog.  The memory leak was 
introduced by commit fbf42729d0e9 ("HID: elo: update the reference count 
of the usb device structure"). not by your commit 817b8b9c5396 ("HID: 
elo: fix memory leak in elo_probe").

Alan Stern

Re: [PATCH] HID: elo: Fix refcount leak in elo_probe()

[Dan Carpenter]

On Fri, Feb 11, 2022 at 08:50:37PM -0500, Alan Stern wrote:
> Syzbot identified a refcount leak in the hid-elo driver:
> 
> BUG: memory leak
> unreferenced object 0xffff88810d49e800 (size 2048):
>   comm "kworker/1:1", pid 25, jiffies 4294954629 (age 16.460s)
>   hex dump (first 32 bytes):
>     ff ff ff ff 31 00 00 00 00 00 00 00 00 00 00 00  ....1...........
>     00 00 00 00 00 00 00 00 00 00 00 00 03 00 00 00  ................
>   backtrace:
>     [<ffffffff82c87a62>] kmalloc include/linux/slab.h:581 [inline]
>     [<ffffffff82c87a62>] kzalloc include/linux/slab.h:715 [inline]
>     [<ffffffff82c87a62>] usb_alloc_dev+0x32/0x450 drivers/usb/core/usb.c:582
>     [<ffffffff82c91a47>] hub_port_connect drivers/usb/core/hub.c:5260 [inline]
>     [<ffffffff82c91a47>] hub_port_connect_change drivers/usb/core/hub.c:5502 [inline]
>     [<ffffffff82c91a47>] port_event drivers/usb/core/hub.c:5660 [inline]
>     [<ffffffff82c91a47>] hub_event+0x1097/0x21a0 drivers/usb/core/hub.c:5742
>     [<ffffffff8126c3ef>] process_one_work+0x2bf/0x600 kernel/workqueue.c:2307
>     [<ffffffff8126ccd9>] worker_thread+0x59/0x5b0 kernel/workqueue.c:2454
>     [<ffffffff81276765>] kthread+0x125/0x160 kernel/kthread.c:377
>     [<ffffffff810022ff>] ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
> 
> Not shown in the bug report but present in the console log:
> 
> [  182.014764][ T3257] elo 0003:04E7:0030.0006: item fetching failed at offset 0/1
> [  182.022255][ T3257] elo 0003:04E7:0030.0006: parse failed
> [  182.027904][ T3257] elo: probe of 0003:04E7:0030.0006 failed with error -22
> [  182.214767][ T3257] usb 1-1: USB disconnect, device number 7
> [  188.090199][ T3604] kmemleak: 3 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
> BUG: memory leak
> 
> which points to hid-elo as the buggy driver.
> 
> The leak is caused by elo_probe() failing to release the reference it
> holds to the struct usb_device in its failure pathway.  In the end the
> driver doesn't need to take this reference at all, because the
> elo_priv structure is always deallocated synchronously when the driver
> unbinds from the interface.
> 
> Therefore this patch fixes the reference leak by not taking the
> reference in the first place.
> 
> Reported-and-tested-by: syzbot+8caaaec4e7a55d75e243@syzkaller.appspotmail.com
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> CC: <stable@vger.kernel.org>
> 

Alan, this bug was fixed a different way in 817b8b9c5396 ("HID: elo: fix
memory leak in elo_probe") so now the two fixes lead to a use after
free.  Your patch is more elegant so we should revert 817b8b9c5396.

regards,
dan carpenter