| Message ID | 20241111120139.3483366-1-snovitoll@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] usb/cdc-wdm: fix memory info leak in wdm_read | expand |
On Mon, Nov 11, 2024 at 05:01:39PM +0500, Sabyrzhan Tasbolatov wrote: > syzbot reported "KMSAN: kernel-infoleak in wdm_read", though there is no > reproducer and the only report for this issue. > > The check: > > if (cntr > count) > cntr = count; > > only limits `cntr` to `count` (the number of bytes requested by > userspace), but it doesn't verify that `desc->ubuf` actually has `count` > bytes. This oversight can lead to situations where `copy_to_user` reads > uninitialized data from `desc->ubuf`. > > This patch makes sure `cntr` respects` both the `desc->length` and the > `count` requested by userspace, preventing any uninitialized memory from > leaking into userspace. > > syzbot report > ============= > BUG: KMSAN: kernel-infoleak in instrument_copy_to_user include/linux/instrumented.h:114 [inline] > BUG: KMSAN: kernel-infoleak in _inline_copy_to_user include/linux/uaccess.h:180 [inline] > BUG: KMSAN: kernel-infoleak in _copy_to_user+0xbc/0x110 lib/usercopy.c:26 > instrument_copy_to_user include/linux/instrumented.h:114 [inline] > _inline_copy_to_user include/linux/uaccess.h:180 [inline] > _copy_to_user+0xbc/0x110 lib/usercopy.c:26 > copy_to_user include/linux/uaccess.h:209 [inline] > wdm_read+0x227/0x1270 drivers/usb/class/cdc-wdm.c:603 > vfs_read+0x2a1/0xf60 fs/read_write.c:474 > ksys_read+0x20f/0x4c0 fs/read_write.c:619 > __do_sys_read fs/read_write.c:629 [inline] > __se_sys_read fs/read_write.c:627 [inline] > __x64_sys_read+0x93/0xe0 fs/read_write.c:627 > x64_sys_call+0x3055/0x3ba0 arch/x86/include/generated/asm/syscalls_64.h:1 > do_syscall_x64 arch/x86/entry/common.c:52 [inline] > do_syscall_64+0xcd/0x1e0 arch/x86/entry/common.c:83 > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > Reported-by: syzbot+9760fbbd535cee131f81@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=9760fbbd535cee131f81 > Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com> > --- > Changes v2 -> v3: > - reverted kzalloc back to kmalloc as the fix is cntr related (Oliver). > - added constraint to select the min length from count and desc->length. > - refactored git commit description as the memory info leak is confirmed. > > Changes v1 -> v2: > - added explanation comment above kzalloc (Greg). > - renamed patch title from memory leak to memory info leak (Greg). > --- > drivers/usb/class/cdc-wdm.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c > index 86ee39db013f..dd7349f8a97a 100644 > --- a/drivers/usb/class/cdc-wdm.c > +++ b/drivers/usb/class/cdc-wdm.c > @@ -598,8 +598,9 @@ static ssize_t wdm_read > spin_unlock_irq(&desc->iuspin); > } > > - if (cntr > count) > - cntr = count; > + /* Ensure cntr does not exceed available data in ubuf. */ > + cntr = min(count, (size_t) desc->length); You should never cast in a call to min(), please use min_t() instead as that is what it is there for. thanks, greg k-h
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c index 86ee39db013f..dd7349f8a97a 100644 --- a/drivers/usb/class/cdc-wdm.c +++ b/drivers/usb/class/cdc-wdm.c @@ -598,8 +598,9 @@ static ssize_t wdm_read spin_unlock_irq(&desc->iuspin); } - if (cntr > count) - cntr = count; + /* Ensure cntr does not exceed available data in ubuf. */ + cntr = min(count, (size_t) desc->length); + rv = copy_to_user(buffer, desc->ubuf, cntr); if (rv > 0) { rv = -EFAULT;
syzbot reported "KMSAN: kernel-infoleak in wdm_read", though there is no reproducer and the only report for this issue. The check: if (cntr > count) cntr = count; only limits `cntr` to `count` (the number of bytes requested by userspace), but it doesn't verify that `desc->ubuf` actually has `count` bytes. This oversight can lead to situations where `copy_to_user` reads uninitialized data from `desc->ubuf`. This patch makes sure `cntr` respects` both the `desc->length` and the `count` requested by userspace, preventing any uninitialized memory from leaking into userspace. syzbot report ============= BUG: KMSAN: kernel-infoleak in instrument_copy_to_user include/linux/instrumented.h:114 [inline] BUG: KMSAN: kernel-infoleak in _inline_copy_to_user include/linux/uaccess.h:180 [inline] BUG: KMSAN: kernel-infoleak in _copy_to_user+0xbc/0x110 lib/usercopy.c:26 instrument_copy_to_user include/linux/instrumented.h:114 [inline] _inline_copy_to_user include/linux/uaccess.h:180 [inline] _copy_to_user+0xbc/0x110 lib/usercopy.c:26 copy_to_user include/linux/uaccess.h:209 [inline] wdm_read+0x227/0x1270 drivers/usb/class/cdc-wdm.c:603 vfs_read+0x2a1/0xf60 fs/read_write.c:474 ksys_read+0x20f/0x4c0 fs/read_write.c:619 __do_sys_read fs/read_write.c:629 [inline] __se_sys_read fs/read_write.c:627 [inline] __x64_sys_read+0x93/0xe0 fs/read_write.c:627 x64_sys_call+0x3055/0x3ba0 arch/x86/include/generated/asm/syscalls_64.h:1 do_syscall_x64 arch/x86/entry/common.c:52 [inline] do_syscall_64+0xcd/0x1e0 arch/x86/entry/common.c:83 entry_SYSCALL_64_after_hwframe+0x77/0x7f Reported-by: syzbot+9760fbbd535cee131f81@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=9760fbbd535cee131f81 Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com> --- Changes v2 -> v3: - reverted kzalloc back to kmalloc as the fix is cntr related (Oliver). - added constraint to select the min length from count and desc->length. - refactored git commit description as the memory info leak is confirmed. Changes v1 -> v2: - added explanation comment above kzalloc (Greg). - renamed patch title from memory leak to memory info leak (Greg). --- drivers/usb/class/cdc-wdm.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)