| Message ID | 20241112132931.3504749-1-snovitoll@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] usb/cdc-wdm: fix memory info leak in wdm_read | expand |
On Tue, Nov 12, 2024 at 06:29:31PM +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. > --- > 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..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); > } Note that the code immediately before the "if" statement which ends here does: cntr = READ_ONCE(desc->length); And the code at the end of the "if" block does: cntr = desc->length; (while holding the spinlock). Thus it is guaranteed that either way, cntr is equal to desc->length when we reach this point. > > - if (cntr > count) > - cntr = count; > + /* Ensure cntr does not exceed available data in ubuf. */ > + cntr = min_t(size_t, count, desc->length); And therefore this line does exactly the same computation as the code you removed. Except for one thing: At this point the spinlock is not held, and your new code does not call READ_ONCE(). That is an oversight. Since the new code does the same thing as the old code, it cannot possibly fix any bugs. (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?) Alan Stern
On Tue, Nov 12, 2024 at 8:52 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Tue, Nov 12, 2024 at 06:29:31PM +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. > > > --- > > 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..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); > > } > > Note that the code immediately before the "if" statement which ends here > does: > > cntr = READ_ONCE(desc->length); > > And the code at the end of the "if" block does: > > cntr = desc->length; > > (while holding the spinlock). Thus it is guaranteed that either way, > cntr is equal to desc->length when we reach this point. > > > > > - if (cntr > count) > > - cntr = count; > > + /* Ensure cntr does not exceed available data in ubuf. */ > > + cntr = min_t(size_t, count, desc->length); > > And therefore this line does exactly the same computation as the code > you removed. Except for one thing: At this point the spinlock is not > held, and your new code does not call READ_ONCE(). That is an > oversight. 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; > > 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); > > (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. > > Alan Stern Please let me know if the diff makes sense and I will proceed with v5. Thanks
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? 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. Alan Stern
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(-)