| Message ID | 20241109152821.3476218-1-snovitoll@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb/cdc-wdm: fix memory leak of wdm_device | expand |
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
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 Regards Oliver
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
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
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;
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(-)