[syzbot] memory leak in hub_event (3)

[syzbot]

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+8caaae...@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

BUG: memory leak
unreferenced object 0xffff88810f5bd660 (size 32):
comm "kworker/1:1", pid 25, jiffies 4294954629 (age 16.460s)
hex dump (first 32 bytes):
31 2d 31 00 00 00 00 00 00 00 00 00 00 00 00 00 1-1.............
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff822cae8c>] kvasprintf+0x6c/0xf0 lib/kasprintf.c:25
[<ffffffff822caf68>] kvasprintf_const+0x58/0x110 lib/kasprintf.c:49
[<ffffffff823c074b>] kobject_set_name_vargs+0x3b/0xe0 lib/kobject.c:289
[<ffffffff826ae353>] dev_set_name+0x63/0x90 drivers/base/core.c:3193
[<ffffffff82c87c20>] usb_alloc_dev+0x1f0/0x450 drivers/usb/core/usb.c:650
[<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

BUG: memory leak
unreferenced object 0xffff888109944200 (size 256):
comm "kworker/1:1", pid 25, jiffies 4294954683 (age 15.920s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 08 42 94 09 81 88 ff ff .........B......
08 42 94 09 81 88 ff ff f0 e3 6a 82 ff ff ff ff .B........j.....
backtrace:
[<ffffffff826b3d9b>] kmalloc include/linux/slab.h:581 [inline]
[<ffffffff826b3d9b>] kzalloc include/linux/slab.h:715 [inline]
[<ffffffff826b3d9b>] device_private_init drivers/base/core.c:3249 [inline]
[<ffffffff826b3d9b>] device_add+0x89b/0xdf0 drivers/base/core.c:3299
[<ffffffff8439de0c>] usb_new_device.cold+0x10f/0x58e drivers/usb/core/hub.c:2566
[<ffffffff82c91d14>] hub_port_connect drivers/usb/core/hub.c:5358 [inline]
[<ffffffff82c91d14>] hub_port_connect_change drivers/usb/core/hub.c:5502 [inline]
[<ffffffff82c91d14>] port_event drivers/usb/core/hub.c:5660 [inline]
[<ffffffff82c91d14>] hub_event+0x1364/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



---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Alan Stern]

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);

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+8caaae...@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.

[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+8caaae...@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <st...@rowland.harvard.edu>
CC: <sta...@vger.kernel.org>

---


[as1971]


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

[Dongliang Mu]

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.

> --
> 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-bug...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/YgcSbUwiALbmoTvL%40rowland.harvard.edu.

[Alan Stern]

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

[Dan Carpenter]

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

[Dan Carpenter]

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

[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

[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

[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

[Greg KH]

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

thanks,

greg k-h

[Dongliang Mu]

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

[Alan Stern]

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

[syzbot]

Auto-closing this bug as obsolete.
No recent activity, existing reproducers are no longer triggering the issue.