| Message ID | 20241112132931.3504749-1-snovitoll@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] usb/cdc-wdm: fix memory info leak in wdm_read | expand |
On Wed, Nov 13, 2024 at 1:25 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, Nov 13, 2024 at 12:30:08AM +0500, Sabyrzhan Tasbolatov wrote: > > I've re-read your and Oliver's comments and come up with this diff, > > which is the same as v4 except it is within a spinlock. > > > > diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c > > index 86ee39db013f..47b299e03e11 100644 > > --- a/drivers/usb/class/cdc-wdm.c > > +++ b/drivers/usb/class/cdc-wdm.c > > @@ -598,8 +598,11 @@ static ssize_t wdm_read > > spin_unlock_irq(&desc->iuspin); > > } > > > > - if (cntr > count) > > - cntr = count; > > + spin_lock_irq(&desc->iuspin); > > + /* Ensure cntr does not exceed available data in ubuf. */ > > + cntr = min_t(size_t, count, desc->length); > > + spin_unlock_irq(&desc->iuspin); > > + > > rv = copy_to_user(buffer, desc->ubuf, cntr); > > if (rv > 0) { > > rv = -EFAULT; > > You seem to be stuck in a rut, doing the same thing over and over again > and not realizing that it accomplishes nothing. The spinlock here > doesn't help; it merely allows you to avoid calling READ_ONCE. > > > > Since the new code does the same thing as the old code, it cannot > > > possibly fix any bugs. > > > > Without the reproducer I can not confirm that this fixes the hypothetical bug, > > however here is my understand how the diff above can fix the memory info leak: > > > > static ssize_t wdm_read() { > > cntr = READ_ONCE(desc->length); > > if (cntr == 0) { > > spin_lock_irq(&desc->iuspin); > > > > /* can remain 0 if not increased in wdm_in_callback() */ > > cntr = desc->length; > > > > spin_unlock_irq(&desc->iuspin); > > } > > > > spin_lock_irq(&desc->iuspin); > > /* take the minimum of whatever user requests `count` and > > desc->length = 0 */ > > cntr = min_t(size_t, count, desc->length); > > spin_lock_irq(&desc->iuspin); > > > > /* cntr is 0, nothing to copy to the user space. */ > > rv = copy_to_user(buffer, desc->ubuf, cntr); > > This does not explain anything. How do you think your change will avoid > the memory info leak? That is, what differences between the old code > and the new code will cause the leak to happen with the old code and not > to happen with your new code? Let me get back to this once I understand how to work with the USB gadgets to emulate a cdc-wdm device to develop a reproducer, because I thought that there is the path to memory info leak and Oliver confirmed it, but now without a solid PoC, I can't proceed further. Sorry for the confusion. > > Note that if cntr is 0 then nothing is copied to user space so there is > no info leak. > > > > (Actually there is one other thing to watch out for: the difference > > > between signed and unsigned values. Here cntr and desc->length are > > > signed whereas count is unsigned. In theory that could cause problems > > > -- it might even be related to the cause of the original bug report. > > > Can you prove that desc->length will never be negative?) > > > > desc->length can not be negative if I understand the following correctly: > > > > static void wdm_in_callback(struct urb *urb) > > { > > ... > > int length = urb->actual_length; > > ... > > if (length + desc->length > desc->wMaxCommand) { > > /* The buffer would overflow */ > > ... > > } else { > > /* we may already be in overflow */ > > if (!test_bit(WDM_OVERFLOW, &desc->flags)) { > > ... > > desc->length += length; > > desc->reslength = length; > > } > > } > > > > urb->actual_length is u32, actually, need to change `int length` to > > `u32 length` though. > > You don't really need to change it. urb->actual_length can never be > larger than urb->length. Ack. > > Alan Stern
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c index 86ee39db013f..5a500973b463 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_t(size_t, count, 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 v3 -> v4: - changed min() to min_t() due to type safety (Greg). 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(-)