linux-kernel - Re: [PATCH v3] usb/cdc-wdm: fix memory info leak in wdm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2024111232-relative-bottom-4995@gregkh>
Date: Tue, 12 Nov 2024 12:13:22 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Sabyrzhan Tasbolatov <snovitoll@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	oneukum@...e.com,
	syzbot+9760fbbd535cee131f81@...kaller.appspotmail.com,
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH v3] usb/cdc-wdm: fix memory info leak in wdm_read

On 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@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9760fbbd535cee131f81
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@...il.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