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@xxxxxxxxxxxxxxxxxxxxxxxxx > Closes: https://syzkaller.appspot.com/bug?extid=9760fbbd535cee131f81 > Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@xxxxxxxxx> > --- > 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
