linux-kernel - Re: [PATCH] usbmon: Fix out-of-bounds read in mon_copy_to

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: <22cdd2ea-9240-45ed-8667-cc315eccf0e0@rowland.harvard.edu>
Date: Wed, 2 Jul 2025 22:29:49 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: contact@...aud-lcm.com
Cc: manas18244@...td.ac.in, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Pete Zaitcev <zaitcev@...hat.com>,
	Paolo Abeni <paolo.abeni@...il.it>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, Greg Kroah-Hartman <gregkh@...e.de>,
	syzbot+86b6d7c8bcc66747c505@...kaller.appspotmail.com
Subject: Re: [PATCH] usbmon: Fix out-of-bounds read in mon_copy_to_buff

On Wed, Jul 02, 2025 at 10:42:42PM +0100, contact@...aud-lcm.com wrote:
> Hi Manas, I just noticed your patch while I was working on it and we had the
> same idea. I think you are not covering every cases of the issue.
> I've done it this way:

Why do both of you guys think that mon_copy_to_buff() is trying to 
transfer data from an empty source buffer?  Maybe the destination buffer 
is the cause of the problem instead.

> From 49f4d231a1c4407d52fee83e956b1d40b3221e37 Mon Sep 17 00:00:00 2001
> From: Arnaud Lecomte <contact@...aud-lcm.com>
> Date: Wed, 2 Jul 2025 22:39:08 +0100
> Subject: [PATCH] usb: mon: Add read length checking in mon_copy_to_buff
> 
> Signed-off-by: Arnaud Lecomte <contact@...aud-lcm.com>
> ---
>  drivers/usb/mon/mon_bin.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index c93b43f5bc46..e7b390439743 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -249,6 +249,8 @@ static unsigned int mon_copy_to_buff(const struct
> mon_reader_bin *this,
>  		 * Copy data and advance pointers.
>  		 */
>  		buf = this->b_vec[off / CHUNK_SIZE].ptr + off % CHUNK_SIZE;
> +		if(strlen(from) < step_len)

strlen() is absolutely the wrong thing to use here for both of your 
patches.  "from" is not a NUL-terminated string but rather a general 
memory buffer.  The caller is supposed to guarantee that "from" contains 
at least "length" bytes of data.  If something is going wrong here, it 
is the caller's fault.

Alan Stern

> +			return -ENOMEM;
>  		memcpy(buf, from, step_len);
>  		if ((off += step_len) >= this->b_size) off = 0;
>  		from += step_len;
> -- 
> 2.43.0
> 
> 
> On 2025-07-02 22:27, Manas Gupta via B4 Relay wrote:
> > From: Manas Gupta <manas18244@...td.ac.in>
> > 
> > memcpy tries to copy buffer 'from' when it is empty. This leads to
> > out-of-bounds crash.
> > 
> > This checks if the buffer is already empty and throws an appropriate
> > error message before bailing out.
> > 
> > Fixes: 6f23ee1fefdc1 ("USB: add binary API to usbmon")
> > Reported-by: syzbot+86b6d7c8bcc66747c505@...kaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=86b6d7c8bcc66747c505
> > Signed-off-by: Manas Gupta <manas18244@...td.ac.in>
> > ---
> > I have used printk(KERN_ERR ... to keep things consistent in usbmon.
> > dev_err and pr_err are not used anywhere else in usbmon.
> > ---
> >  drivers/usb/mon/mon_bin.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> > index c93b43f5bc4614ad75568b601c47a1ae51f01fa5..bd945052f6fb821ba814fab96eba5a82e5d161e2
> > 100644
> > --- a/drivers/usb/mon/mon_bin.c
> > +++ b/drivers/usb/mon/mon_bin.c
> > @@ -249,6 +249,11 @@ static unsigned int mon_copy_to_buff(const struct
> > mon_reader_bin *this,
> >  		 * Copy data and advance pointers.
> >  		 */
> >  		buf = this->b_vec[off / CHUNK_SIZE].ptr + off % CHUNK_SIZE;
> > +		if (!strlen(from)) {
> > +			printk(KERN_ERR TAG
> > +			       ": src buffer is empty, cannot copy from it\n");
> > +			return -ENOMEM;
> > +		}
> >  		memcpy(buf, from, step_len);
> >  		if ((off += step_len) >= this->b_size) off = 0;
> >  		from += step_len;
> > 
> > ---
> > base-commit: d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af
> > change-id: 20250703-fix-oob-mon_copy_to_buff-7cfe26e819b9
> > 
> > Best regards,
>