Re: [PATCH] usb/cdc-wdm: fix memory leak of wdm_device

[Greg KH <gregkh@linuxfoundation.org>]

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