drivers/usb/class/cdc-wdm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
syzbot reported "KMSAN: kernel-infoleak in wdm_read", though there is no
reproducer and the only report for this issue. This might be
a false-positive, but while the reading the code, it seems,
there is the way to leak kernel memory.
Here what I understand so far from the report happening
with ubuf in drivers/usb/class/cdc-wdm.c:
1. kernel buffer "ubuf" is allocated during cdc-wdm device creation in
the "struct wdm_device":
static int wdm_create()
{
...
desc->ubuf = kmalloc(desc->wMaxCommand, GFP_KERNEL);
...
usb_fill_control_urb(
...
wdm_in_callback,
...
);
}
2. during wdm_create() it calls wdm_in_callback() which MAY fill "ubuf"
for the first time via memmove if conditions are met.
static void wdm_in_callback()
{
...
if (length + desc->length > desc->wMaxCommand) {
...
} else {
/* we may already be in overflow */
if (!test_bit(WDM_OVERFLOW, &desc->flags)) {
memmove(desc->ubuf + desc->length, desc->inbuf, length);
desc->length += length;
desc->reslength = length;
}
}
...
}
3. if conditions are not fulfilled in step 2., then calling read() syscall
which calls wdm_read(), should leak the random kernel memory via
copy_to_user() from "ubuf" buffer which is allocated in kmalloc-256.
static ssize_t wdm_read()
{
...
struct wdm_device *desc = file->private_data;
cntr = READ_ONCE(desc->length);
...
if (cntr > count)
cntr = count;
rv = copy_to_user(buffer, desc->ubuf, cntr);
...
}
, where wMaxCommand is 256, AFAIU.
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>
---
drivers/usb/class/cdc-wdm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 86ee39db013f..8801e03196de 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -1063,7 +1063,7 @@ static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor
if (!desc->command)
goto err;
- desc->ubuf = kmalloc(desc->wMaxCommand, GFP_KERNEL);
+ desc->ubuf = kzalloc(desc->wMaxCommand, GFP_KERNEL);
if (!desc->ubuf)
goto err;
--
2.34.1On 09.11.24 16:28, Sabyrzhan Tasbolatov wrote:
Hi,
> syzbot reported "KMSAN: kernel-infoleak in wdm_read", though there is no
> reproducer and the only report for this issue. This might be
> a false-positive, but while the reading the code, it seems,
> there is the way to leak kernel memory.
As far as I can tell, the leak is real.
> Here what I understand so far from the report happening
> with ubuf in drivers/usb/class/cdc-wdm.c:
>
> 1. kernel buffer "ubuf" is allocated during cdc-wdm device creation in
> the "struct wdm_device":
Yes
[..]
> 2. during wdm_create() it calls wdm_in_callback() which MAY fill "ubuf"
> for the first time via memmove if conditions are met.
Yes.
[..]
> 3. if conditions are not fulfilled in step 2., then calling read() syscall
> which calls wdm_read(), should leak the random kernel memory via
> copy_to_user() from "ubuf" buffer which is allocated in kmalloc-256.
Yes, sort of.
>
> - desc->ubuf = kmalloc(desc->wMaxCommand, GFP_KERNEL);
> + desc->ubuf = kzalloc(desc->wMaxCommand, GFP_KERNEL);
> if (!desc->ubuf)
> goto err;
No. I am sorry, but the fix is wrong. Absolutely wrong.
Let's look at the code of wdm_read():
cntr = desc->length;
Here the method determines how much data is in the buffer.
"length" initially is zero, because the descriptor itself
is allocated with kzalloc. It is increased in the callback.
spin_unlock_irq(&desc->iuspin);
}
if (cntr > count)
cntr = count;
This is _supposed_ to make sure that user space does not get more
than we have in the buffer.
rv = copy_to_user(buffer, desc->ubuf, cntr);
if (rv > 0) {
rv = -EFAULT;
goto err;
}
spin_lock_irq(&desc->iuspin);
for (i = 0; i < desc->length - cntr; i++)
desc->ubuf[i] = desc->ubuf[i + cntr];
desc->length -= cntr;
Here we decrease the count of what we have in the buffer.
Now please look at the check again
"cntr" is what we have in the buffer.
"count" is how much user space wants.
We should limit what we copy to the amount we have in the buffer.
But that is not what the check does. Instead it makes sure we never
copy more than user space requested. But we do not check whether
the buffer has enough data to satisfy the read.
You have discovered the bug. If you want to propose a fix, the honor is yours.
Or do you want me to fix it?
tl;dr: Excellent catch, wrong fix
Regards
Oliver
On Mon, Nov 11, 2024 at 10:44:43AM +0100, Oliver Neukum wrote:
> On 09.11.24 16:28, Sabyrzhan Tasbolatov wrote:
>
> Hi,
>
> > syzbot reported "KMSAN: kernel-infoleak in wdm_read", though there is no
> > reproducer and the only report for this issue. This might be
> > a false-positive, but while the reading the code, it seems,
> > there is the way to leak kernel memory.
>
> As far as I can tell, the leak is real.
>
> > Here what I understand so far from the report happening
> > with ubuf in drivers/usb/class/cdc-wdm.c:
> >
> > 1. kernel buffer "ubuf" is allocated during cdc-wdm device creation in
> > the "struct wdm_device":
>
> Yes
> [..]
>
> > 2. during wdm_create() it calls wdm_in_callback() which MAY fill "ubuf"
> > for the first time via memmove if conditions are met.
>
> Yes.
> [..]
>
> > 3. if conditions are not fulfilled in step 2., then calling read() syscall
> > which calls wdm_read(), should leak the random kernel memory via
> > copy_to_user() from "ubuf" buffer which is allocated in kmalloc-256.
>
> Yes, sort of.
>
> > - desc->ubuf = kmalloc(desc->wMaxCommand, GFP_KERNEL);
> > + desc->ubuf = kzalloc(desc->wMaxCommand, GFP_KERNEL);
> > if (!desc->ubuf)
> > goto err;
>
> No. I am sorry, but the fix is wrong. Absolutely wrong.
>
> Let's look at the code of wdm_read():
>
> cntr = desc->length;
> Here the method determines how much data is in the buffer.
> "length" initially is zero, because the descriptor itself
> is allocated with kzalloc. It is increased in the callback.
>
> spin_unlock_irq(&desc->iuspin);
> }
>
> if (cntr > count)
> cntr = count;
>
> This is _supposed_ to make sure that user space does not get more
> than we have in the buffer.
>
> rv = copy_to_user(buffer, desc->ubuf, cntr);
> if (rv > 0) {
> rv = -EFAULT;
> goto err;
> }
>
> spin_lock_irq(&desc->iuspin);
>
> for (i = 0; i < desc->length - cntr; i++)
> desc->ubuf[i] = desc->ubuf[i + cntr];
>
> desc->length -= cntr;
>
> Here we decrease the count of what we have in the buffer.
>
> Now please look at the check again
>
> "cntr" is what we have in the buffer.
> "count" is how much user space wants.
>
> We should limit what we copy to the amount we have in the buffer.
> But that is not what the check does. Instead it makes sure we never
> copy more than user space requested. But we do not check whether
> the buffer has enough data to satisfy the read.
I don't understand your analysis. As you said, cntr is initially set to
the amount in the buffer:
If cntr <= count then cntr isn't changed, so the amount of data
copied to the user is the same as what is in the buffer.
Otherwise, if cntr > count, then cntr is decreased so that the
amount copied to the user is no larger than what the user asked
for -- but then it's obviously smaller than what's in the buffer.
In neither case does the code copy more data than the buffer contains.
Alan Stern
On Mon, Nov 11, 2024 at 7:29 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, Nov 11, 2024 at 10:44:43AM +0100, Oliver Neukum wrote:
> > On 09.11.24 16:28, Sabyrzhan Tasbolatov wrote:
> >
> > Hi,
> >
> > > syzbot reported "KMSAN: kernel-infoleak in wdm_read", though there is no
> > > reproducer and the only report for this issue. This might be
> > > a false-positive, but while the reading the code, it seems,
> > > there is the way to leak kernel memory.
> >
> > As far as I can tell, the leak is real.
> >
> > > Here what I understand so far from the report happening
> > > with ubuf in drivers/usb/class/cdc-wdm.c:
> > >
> > > 1. kernel buffer "ubuf" is allocated during cdc-wdm device creation in
> > > the "struct wdm_device":
> >
> > Yes
> > [..]
> >
> > > 2. during wdm_create() it calls wdm_in_callback() which MAY fill "ubuf"
> > > for the first time via memmove if conditions are met.
> >
> > Yes.
> > [..]
> >
> > > 3. if conditions are not fulfilled in step 2., then calling read() syscall
> > > which calls wdm_read(), should leak the random kernel memory via
> > > copy_to_user() from "ubuf" buffer which is allocated in kmalloc-256.
> >
> > Yes, sort of.
> >
> > > - desc->ubuf = kmalloc(desc->wMaxCommand, GFP_KERNEL);
> > > + desc->ubuf = kzalloc(desc->wMaxCommand, GFP_KERNEL);
> > > if (!desc->ubuf)
> > > goto err;
> >
> > No. I am sorry, but the fix is wrong. Absolutely wrong.
> >
> > Let's look at the code of wdm_read():
> >
> > cntr = desc->length;
> > Here the method determines how much data is in the buffer.
> > "length" initially is zero, because the descriptor itself
> > is allocated with kzalloc. It is increased in the callback.
> >
> > spin_unlock_irq(&desc->iuspin);
> > }
> >
> > if (cntr > count)
> > cntr = count;
> >
> > This is _supposed_ to make sure that user space does not get more
> > than we have in the buffer.
> >
> > rv = copy_to_user(buffer, desc->ubuf, cntr);
> > if (rv > 0) {
> > rv = -EFAULT;
> > goto err;
> > }
> >
> > spin_lock_irq(&desc->iuspin);
> >
> > for (i = 0; i < desc->length - cntr; i++)
> > desc->ubuf[i] = desc->ubuf[i + cntr];
> >
> > desc->length -= cntr;
> >
> > Here we decrease the count of what we have in the buffer.
> >
> > Now please look at the check again
> >
> > "cntr" is what we have in the buffer.
> > "count" is how much user space wants.
> >
> > We should limit what we copy to the amount we have in the buffer.
> > But that is not what the check does. Instead it makes sure we never
> > copy more than user space requested. But we do not check whether
> > the buffer has enough data to satisfy the read.
>
> I don't understand your analysis. As you said, cntr is initially set to
> the amount in the buffer:
>
> If cntr <= count then cntr isn't changed, so the amount of data
> copied to the user is the same as what is in the buffer.
>
> Otherwise, if cntr > count, then cntr is decreased so that the
> amount copied to the user is no larger than what the user asked
> for -- but then it's obviously smaller than what's in the buffer.
>
> In neither case does the code copy more data than the buffer contains.
Hello,
I've sent the v3 patch [1] per Oliver's explanation if I interpreted
it correctly.
I don't have the reproducer to verify if the patch solves the problem.
If the analysis or patch is not right, please let me know.
[1] https://lore.kernel.org/all/20241111120139.3483366-1-snovitoll@gmail.com/
>
> Alan Stern
On Tue, Nov 12, 2024 at 02:34:13PM +0500, Sabyrzhan Tasbolatov wrote: > On Mon, Nov 11, 2024 at 7:29 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > I don't understand your analysis. As you said, cntr is initially set to > > the amount in the buffer: > > > > If cntr <= count then cntr isn't changed, so the amount of data > > copied to the user is the same as what is in the buffer. > > > > Otherwise, if cntr > count, then cntr is decreased so that the > > amount copied to the user is no larger than what the user asked > > for -- but then it's obviously smaller than what's in the buffer. > > > > In neither case does the code copy more data than the buffer contains. > > Hello, > I've sent the v3 patch [1] per Oliver's explanation if I interpreted > it correctly. > I don't have the reproducer to verify if the patch solves the problem. > If the analysis or patch is not right, please let me know. The analysis is not right. The patch is also not right, because it doesn't change the meaning of the code (except for one respect, in which it is wrong). I'll send another email responding to the patch itself. Alan Stern
On Mon, Nov 11, 2024 at 2:44 PM Oliver Neukum <oneukum@suse.com> wrote:
>
> On 09.11.24 16:28, Sabyrzhan Tasbolatov wrote:
>
> Hi,
>
> > syzbot reported "KMSAN: kernel-infoleak in wdm_read", though there is no
> > reproducer and the only report for this issue. This might be
> > a false-positive, but while the reading the code, it seems,
> > there is the way to leak kernel memory.
>
> As far as I can tell, the leak is real.
>
> > Here what I understand so far from the report happening
> > with ubuf in drivers/usb/class/cdc-wdm.c:
> >
> > 1. kernel buffer "ubuf" is allocated during cdc-wdm device creation in
> > the "struct wdm_device":
>
> Yes
> [..]
>
> > 2. during wdm_create() it calls wdm_in_callback() which MAY fill "ubuf"
> > for the first time via memmove if conditions are met.
>
> Yes.
> [..]
>
> > 3. if conditions are not fulfilled in step 2., then calling read() syscall
> > which calls wdm_read(), should leak the random kernel memory via
> > copy_to_user() from "ubuf" buffer which is allocated in kmalloc-256.
>
> Yes, sort of.
>
> >
> > - desc->ubuf = kmalloc(desc->wMaxCommand, GFP_KERNEL);
> > + desc->ubuf = kzalloc(desc->wMaxCommand, GFP_KERNEL);
> > if (!desc->ubuf)
> > goto err;
>
> No. I am sorry, but the fix is wrong. Absolutely wrong.
>
> Let's look at the code of wdm_read():
>
> cntr = desc->length;
> Here the method determines how much data is in the buffer.
> "length" initially is zero, because the descriptor itself
> is allocated with kzalloc. It is increased in the callback.
>
> spin_unlock_irq(&desc->iuspin);
> }
>
> if (cntr > count)
> cntr = count;
>
> This is _supposed_ to make sure that user space does not get more
> than we have in the buffer.
>
> rv = copy_to_user(buffer, desc->ubuf, cntr);
> if (rv > 0) {
> rv = -EFAULT;
> goto err;
> }
>
> spin_lock_irq(&desc->iuspin);
>
> for (i = 0; i < desc->length - cntr; i++)
> desc->ubuf[i] = desc->ubuf[i + cntr];
>
> desc->length -= cntr;
>
> Here we decrease the count of what we have in the buffer.
>
> Now please look at the check again
>
> "cntr" is what we have in the buffer.
> "count" is how much user space wants.
>
> We should limit what we copy to the amount we have in the buffer.
> But that is not what the check does. Instead it makes sure we never
> copy more than user space requested. But we do not check whether
> the buffer has enough data to satisfy the read.
>
> You have discovered the bug. If you want to propose a fix, the honor is yours.
> Or do you want me to fix it?
>
> tl;dr: Excellent catch, wrong fix
Hi, thanks for the comments.
Let me try to come up with a fix with your explanation in a few hours,
this will help me understand this bug research as the complete experience.
BTW, I couldn't make a reproduction to create /dev/cdc-wdm0 device to
read from it
via dummy_hdc, USB raw gadget (learning in this part as well), to
verify the fix.
I also wanted to request a CVE for my CV
after the fix is released per kernel.org CNA rules :) but it's not so important.
>
> Regards
> Oliver
>
On Mon, Nov 11, 2024 at 03:40:29PM +0500, Sabyrzhan Tasbolatov wrote: > On Mon, Nov 11, 2024 at 2:44 PM Oliver Neukum <oneukum@suse.com> wrote: > I also wanted to request a CVE for my CV > after the fix is released per kernel.org CNA rules :) but it's not so important. Should not be a problem, but has nothing to do with this fix at the moment :) thanks, greg k-h
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);
+
rv = copy_to_user(buffer, desc->ubuf, cntr);
if (rv > 0) {
rv = -EFAULT;
--
2.34.1On 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
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(-)
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;
--
2.34.1On 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
On Sat, Nov 09, 2024 at 08:28:21PM +0500, Sabyrzhan Tasbolatov wrote:
> syzbot reported "KMSAN: kernel-infoleak in wdm_read", though there is no
> reproducer and the only report for this issue. This might be
> a false-positive, but while the reading the code, it seems,
> there is the way to leak kernel memory.
Nit, the subject should say "memory info leak" as traditionally "memory
leak" means that we loose memory, not expose random stuff to userspace
:)
> Here what I understand so far from the report happening
> with ubuf in drivers/usb/class/cdc-wdm.c:
>
> 1. kernel buffer "ubuf" is allocated during cdc-wdm device creation in
> the "struct wdm_device":
>
> static int wdm_create()
> {
> ...
> desc->ubuf = kmalloc(desc->wMaxCommand, GFP_KERNEL);
> ...
> usb_fill_control_urb(
> ...
> wdm_in_callback,
> ...
> );
>
> }
>
> 2. during wdm_create() it calls wdm_in_callback() which MAY fill "ubuf"
> for the first time via memmove if conditions are met.
>
> static void wdm_in_callback()
> {
> ...
> if (length + desc->length > desc->wMaxCommand) {
> ...
> } else {
> /* we may already be in overflow */
> if (!test_bit(WDM_OVERFLOW, &desc->flags)) {
> memmove(desc->ubuf + desc->length, desc->inbuf, length);
> desc->length += length;
> desc->reslength = length;
> }
> }
> ...
> }
>
> 3. if conditions are not fulfilled in step 2., then calling read() syscall
> which calls wdm_read(), should leak the random kernel memory via
> copy_to_user() from "ubuf" buffer which is allocated in kmalloc-256.
>
> static ssize_t wdm_read()
> {
> ...
> struct wdm_device *desc = file->private_data;
> cntr = READ_ONCE(desc->length);
> ...
> if (cntr > count)
> cntr = count;
> rv = copy_to_user(buffer, desc->ubuf, cntr);
> ...
> }
>
> , where wMaxCommand is 256, AFAIU.
>
> 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>
> ---
> drivers/usb/class/cdc-wdm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index 86ee39db013f..8801e03196de 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -1063,7 +1063,7 @@ static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor
> if (!desc->command)
> goto err;
>
> - desc->ubuf = kmalloc(desc->wMaxCommand, GFP_KERNEL);
> + desc->ubuf = kzalloc(desc->wMaxCommand, GFP_KERNEL);
Seems good, but can you put a comment above this saying "need to zero
this out because it could expose data to userspace"
thanks,
greg k-h
syzbot reported "KMSAN: kernel-infoleak in wdm_read", though there is no
reproducer and the only report for this issue. This might be
a false-positive, but while the reading the code, it seems,
there is the way to leak kernel memory.
Here what I understand so far from the report happening
with ubuf in drivers/usb/class/cdc-wdm.c:
1. kernel buffer "ubuf" is allocated during cdc-wdm device creation in
the "struct wdm_device":
static int wdm_create()
{
...
desc->ubuf = kmalloc(desc->wMaxCommand, GFP_KERNEL);
...
usb_fill_control_urb(
...
wdm_in_callback,
...
);
}
2. during wdm_create() it calls wdm_in_callback() which MAY fill "ubuf"
for the first time via memmove if conditions are met.
static void wdm_in_callback()
{
...
if (length + desc->length > desc->wMaxCommand) {
...
} else {
/* we may already be in overflow */
if (!test_bit(WDM_OVERFLOW, &desc->flags)) {
memmove(desc->ubuf + desc->length, desc->inbuf, length);
desc->length += length;
desc->reslength = length;
}
}
...
}
3. if conditions are not fulfilled in step 2., then calling read() syscall
which calls wdm_read(), should leak the random kernel memory via
copy_to_user() from "ubuf" buffer which is allocated in kmalloc-256.
static ssize_t wdm_read()
{
...
struct wdm_device *desc = file->private_data;
cntr = READ_ONCE(desc->length);
...
if (cntr > count)
cntr = count;
rv = copy_to_user(buffer, desc->ubuf, cntr);
...
}
, where wMaxCommand is 256, AFAIU.
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 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 | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 86ee39db013f..59176e91ff9b 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -1063,7 +1063,8 @@ static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor
if (!desc->command)
goto err;
- desc->ubuf = kmalloc(desc->wMaxCommand, GFP_KERNEL);
+ /* Need to zero this out because it could expose data to userspace. */
+ desc->ubuf = kzalloc(desc->wMaxCommand, GFP_KERNEL);
if (!desc->ubuf)
goto err;
--
2.34.1© 2016 - 2024 Red Hat, Inc.