[syzbot] [usb?] memory leak in uss720_probe

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 58717b2a1365 Merge tag 'sound-7.2-rc4' of git://git.kernel..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=15a344b9580000
kernel config: https://syzkaller.appspot.com/x/.config?x=b80376788da8bcda
dashboard link: https://syzkaller.appspot.com/bug?extid=56962eb32ba0136cd330
compiler: gcc (Debian 14.2.0-19) 14.2.0, GNU ld (GNU Binutils for Debian) 2.44
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=10f70b46580000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=162454b9580000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/dac88573d763/disk-58717b2a.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/29447c97aeb1/vmlinux-58717b2a.xz
kernel image: https://storage.googleapis.com/syzbot-assets/076ab0265b89/bzImage-58717b2a.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/cbc20556ddac/mount_1.gz
fsck result: failed (log: https://syzkaller.appspot.com/x/fsck.log?x=17770b46580000)

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+56962e...@syzkaller.appspotmail.com

BUG: memory leak
unreferenced object 0xffff88812c0e34c0 (size 64):
comm "kworker/0:0", pid 9, jiffies 4294942439
hex dump (first 32 bytes):
00 b8 b6 05 81 88 ff ff 00 00 00 00 00 00 00 00 ................
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace (crc 7291ab94):
kmemleak_alloc_recursive include/linux/kmemleak.h:44 [inline]
slab_post_alloc_hook mm/slub.c:4625 [inline]
slab_alloc_node mm/slub.c:4945 [inline]
__kmalloc_cache_noprof+0x356/0x440 mm/slub.c:5511
_kmalloc_noprof include/linux/slab.h:969 [inline]
_kzalloc_noprof include/linux/slab.h:1290 [inline]
uss720_probe+0xf6/0x330 drivers/usb/misc/uss720.c:702
usb_probe_interface+0x210/0x480 drivers/usb/core/driver.c:396
call_driver_probe drivers/base/dd.c:628 [inline]
really_probe+0x12f/0x3a0 drivers/base/dd.c:706
__driver_probe_device+0xe9/0x1a0 drivers/base/dd.c:868
driver_probe_device+0x28/0xf0 drivers/base/dd.c:898
__device_attach_driver+0x10f/0x170 drivers/base/dd.c:1026
bus_for_each_drv+0xd2/0x130 drivers/base/bus.c:500
__device_attach+0xf9/0x290 drivers/base/dd.c:1098
device_initial_probe+0x6f/0x80 drivers/base/dd.c:1153
bus_probe_device+0x3e/0xb0 drivers/base/bus.c:620
device_add+0x99f/0xc80 drivers/base/core.c:3772
usb_set_configuration+0x8f5/0xb80 drivers/usb/core/message.c:2268
usb_generic_driver_probe+0x73/0xb0 drivers/usb/core/generic.c:250
usb_probe_device+0x78/0x1f0 drivers/usb/core/driver.c:291
call_driver_probe drivers/base/dd.c:628 [inline]
really_probe+0x12f/0x3a0 drivers/base/dd.c:706

BUG: memory leak
unreferenced object 0xffff88812d350b40 (size 96):
comm "kworker/0:0", pid 9, jiffies 4294942439
hex dump (first 32 bytes):
c0 34 0e 2c 81 88 ff ff 01 00 00 00 00 00 00 00 .4.,............
e0 34 0e 2c 81 88 ff ff e0 34 0e 2c 81 88 ff ff .4.,.....4.,....
backtrace (crc 519c4231):
kmemleak_alloc_recursive include/linux/kmemleak.h:44 [inline]
slab_post_alloc_hook mm/slub.c:4625 [inline]
slab_alloc_node mm/slub.c:4945 [inline]
__kmalloc_cache_noprof+0x356/0x440 mm/slub.c:5511
_kmalloc_noprof include/linux/slab.h:969 [inline]
_kzalloc_noprof include/linux/slab.h:1290 [inline]
submit_async_request.constprop.0+0x6b/0x440 drivers/usb/misc/uss720.c:137
get_1284_register+0x4a/0x1a0 drivers/usb/misc/uss720.c:205
uss720_probe+0x205/0x330 drivers/usb/misc/uss720.c:733
usb_probe_interface+0x210/0x480 drivers/usb/core/driver.c:396
call_driver_probe drivers/base/dd.c:628 [inline]
really_probe+0x12f/0x3a0 drivers/base/dd.c:706
__driver_probe_device+0xe9/0x1a0 drivers/base/dd.c:868
driver_probe_device+0x28/0xf0 drivers/base/dd.c:898
__device_attach_driver+0x10f/0x170 drivers/base/dd.c:1026
bus_for_each_drv+0xd2/0x130 drivers/base/bus.c:500
__device_attach+0xf9/0x290 drivers/base/dd.c:1098
device_initial_probe+0x6f/0x80 drivers/base/dd.c:1153
bus_probe_device+0x3e/0xb0 drivers/base/bus.c:620
device_add+0x99f/0xc80 drivers/base/core.c:3772
usb_set_configuration+0x8f5/0xb80 drivers/usb/core/message.c:2268
usb_generic_driver_probe+0x73/0xb0 drivers/usb/core/generic.c:250

BUG: memory leak
unreferenced object 0xffff88812d277180 (size 192):
comm "kworker/0:0", pid 9, jiffies 4294942439
hex dump (first 32 bytes):
01 00 00 00 98 ff ff ff 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 98 71 27 2d 81 88 ff ff .........q'-....
backtrace (crc 26b7be33):
kmemleak_alloc_recursive include/linux/kmemleak.h:44 [inline]
slab_post_alloc_hook mm/slub.c:4625 [inline]
slab_alloc_node mm/slub.c:4945 [inline]
__do_kmalloc_node mm/slub.c:5361 [inline]
__kmalloc_noprof+0x3bf/0x550 mm/slub.c:5387
_kmalloc_noprof include/linux/slab.h:973 [inline]
usb_alloc_urb+0x66/0xa0 drivers/usb/core/urb.c:75
submit_async_request.constprop.0+0x11c/0x440 drivers/usb/misc/uss720.c:145
get_1284_register+0x4a/0x1a0 drivers/usb/misc/uss720.c:205
uss720_probe+0x205/0x330 drivers/usb/misc/uss720.c:733
usb_probe_interface+0x210/0x480 drivers/usb/core/driver.c:396
call_driver_probe drivers/base/dd.c:628 [inline]
really_probe+0x12f/0x3a0 drivers/base/dd.c:706
__driver_probe_device+0xe9/0x1a0 drivers/base/dd.c:868
driver_probe_device+0x28/0xf0 drivers/base/dd.c:898
__device_attach_driver+0x10f/0x170 drivers/base/dd.c:1026
bus_for_each_drv+0xd2/0x130 drivers/base/bus.c:500
__device_attach+0xf9/0x290 drivers/base/dd.c:1098
device_initial_probe+0x6f/0x80 drivers/base/dd.c:1153
bus_probe_device+0x3e/0xb0 drivers/base/bus.c:620
device_add+0x99f/0xc80 drivers/base/core.c:3772
usb_set_configuration+0x8f5/0xb80 drivers/usb/core/message.c:2268

BUG: memory leak
unreferenced object 0xffff888111cfec80 (size 8):
comm "kworker/0:0", pid 9, jiffies 4294942439
hex dump (first 8 bytes):
c0 03 00 00 00 00 07 00 ........
backtrace (crc 40fcb6d):
kmemleak_alloc_recursive include/linux/kmemleak.h:44 [inline]
slab_post_alloc_hook mm/slub.c:4625 [inline]
slab_alloc_node mm/slub.c:4945 [inline]
__kmalloc_cache_noprof+0x356/0x440 mm/slub.c:5511
_kmalloc_noprof include/linux/slab.h:969 [inline]
submit_async_request.constprop.0+0x143/0x440 drivers/usb/misc/uss720.c:150
get_1284_register+0x4a/0x1a0 drivers/usb/misc/uss720.c:205
uss720_probe+0x205/0x330 drivers/usb/misc/uss720.c:733
usb_probe_interface+0x210/0x480 drivers/usb/core/driver.c:396
call_driver_probe drivers/base/dd.c:628 [inline]
really_probe+0x12f/0x3a0 drivers/base/dd.c:706
__driver_probe_device+0xe9/0x1a0 drivers/base/dd.c:868
driver_probe_device+0x28/0xf0 drivers/base/dd.c:898
__device_attach_driver+0x10f/0x170 drivers/base/dd.c:1026
bus_for_each_drv+0xd2/0x130 drivers/base/bus.c:500
__device_attach+0xf9/0x290 drivers/base/dd.c:1098
device_initial_probe+0x6f/0x80 drivers/base/dd.c:1153
bus_probe_device+0x3e/0xb0 drivers/base/bus.c:620
device_add+0x99f/0xc80 drivers/base/core.c:3772
usb_set_configuration+0x8f5/0xb80 drivers/usb/core/message.c:2268
usb_generic_driver_probe+0x73/0xb0 drivers/usb/core/generic.c:250

connection error: failed to recv *flatrpc.ExecutorMessageRawT: EOF


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

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[syzbot]

For archival purposes, forwarding an incoming command email to
linux-...@vger.kernel.org, syzkall...@googlegroups.com.

***

Subject: [PATCH] usb: misc: uss720: fix memory leak by replacing usb_unlink_urb with usb_kill_urb
Author: karti...@gmail.com

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master


kmemleak reported memory leaks in uss720_probe() during device
initialization. The leaks occur because kill_all_async_requests_priv()
was using usb_unlink_urb() which is asynchronous and returns immediately
without waiting for the URB completion callbacks to finish.

This caused a race condition where kref_put() in the probe_abort path
would run before async_complete() had a chance to call destroy_async(),
leaving the URB, request struct (uss720_async_request), and transfer
buffer (dr) unreferenced and leaked.

Fix this by replacing usb_unlink_urb() with usb_kill_urb() which blocks
until the completion callback has finished, ensuring destroy_async()
runs and frees all associated memory before kref_put() is called on
priv.

Since usb_kill_urb() may sleep, it cannot be called while holding a
spinlock. Drop and reacquire the spinlock around each usb_kill_urb()
call. Also switch to list_for_each_entry_safe() to safely handle list
modification during iteration, as destroy_async() calls list_del_init()
on the current entry.

Reported-by: syzbot+56962e...@syzkaller.appspotmail.com
Closes: http://syzkaller.appspot.com/bug?extid=56962eb32ba0136cd330
Signed-off-by: Deepanshu Kartikey <karti...@gmail.com>
---
drivers/usb/misc/uss720.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/misc/uss720.c b/drivers/usb/misc/uss720.c
index 1ce48f5832d7..f43fbb28044b 100644
--- a/drivers/usb/misc/uss720.c
+++ b/drivers/usb/misc/uss720.c
@@ -175,13 +175,15 @@ static struct uss720_async_request *submit_async_request(struct parport_uss720_p

static unsigned int kill_all_async_requests_priv(struct parport_uss720_private *priv)
{
- struct uss720_async_request *rq;
+ struct uss720_async_request *rq, *tmp;
unsigned long flags;
unsigned int ret = 0;

spin_lock_irqsave(&priv->asynclock, flags);
- list_for_each_entry(rq, &priv->asynclist, asynclist) {
- usb_unlink_urb(rq->urb);
+ list_for_each_entry_safe(rq, tmp, &priv->asynclist, asynclist) {
+ spin_unlock_irqrestore(&priv->asynclock, flags);
+ usb_kill_urb(rq->urb);
+ spin_lock_irqsave(&priv->asynclock, flags);
ret++;
}
spin_unlock_irqrestore(&priv->asynclock, flags);
--
2.43.0

[David Laight]

I'll take a bet on that not being correct.
The _safe() only lets you remove the current item.
So if the list can be changed by someone else it doesn't help.
Possibly this is tidy up code and the lock just isn't needed?
Maybe the head of the list should be removed under the lock?
(and the rest of the tidy up done at the same time.)

David

> ret++;
> }
> spin_unlock_irqrestore(&priv->asynclock, flags);