[PATCH] usbmon: Fix out-of-bounds read in mon_copy_to_buff

[PATCH] usbmon: Fix out-of-bounds read in mon_copy_to_buff

[Manas Gupta via B4 Relay]

From: Manas Gupta <manas18244@iiitd.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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=86b6d7c8bcc66747c505
Signed-off-by: Manas Gupta <manas18244@iiitd.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,
-- 
Manas Gupta <manas18244@iiitd.ac.in>

Re: [PATCH] usbmon: Fix out-of-bounds read in mon_copy_to_buff

[contact]

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:
 From 49f4d231a1c4407d52fee83e956b1d40b3221e37 Mon Sep 17 00:00:00 2001
 From: Arnaud Lecomte <contact@arnaud-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@arnaud-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)
+			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@iiitd.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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=86b6d7c8bcc66747c505
> Signed-off-by: Manas Gupta <manas18244@iiitd.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,

Re: [PATCH] usbmon: Fix out-of-bounds read in mon_copy_to_buff

[Alan Stern]

On Wed, Jul 02, 2025 at 10:42:42PM +0100, contact@arnaud-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@arnaud-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@arnaud-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@iiitd.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@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=86b6d7c8bcc66747c505
> > Signed-off-by: Manas Gupta <manas18244@iiitd.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,
> 

Re: [PATCH] usbmon: Fix out-of-bounds read in mon_copy_to_buff

[Greg Kroah-Hartman]

On Thu, Jul 03, 2025 at 02:57:40AM +0530, Manas Gupta via B4 Relay wrote:
> From: Manas Gupta <manas18244@iiitd.ac.in>
> 
> memcpy tries to copy buffer 'from' when it is empty. This leads to
> out-of-bounds crash.

What exactly is the "crash" that you are seeing?  What is reporting it,
and how?

thanks,

greg k-h

Re: [PATCH] usbmon: Fix out-of-bounds read in mon_copy_to_buff

[Manas]

On 03.07.2025 07:12, Greg Kroah-Hartman wrote:
>On Thu, Jul 03, 2025 at 02:57:40AM +0530, Manas Gupta via B4 Relay wrote:
>> From: Manas Gupta <manas18244@iiitd.ac.in>
>>
>> memcpy tries to copy buffer 'from' when it is empty. This leads to
>> out-of-bounds crash.
>
>What exactly is the "crash" that you are seeing?  What is reporting it,
>and how?
>
Hi Greg and Alan,

I ran the reproducer[1] on my machine and got the following stacktrace.

```
[   41.601410][  T769] ==================================================================
[   41.601908][  T769] BUG: KASAN: slab-out-of-bounds in mon_copy_to_buff+0xc6/0x180
[   41.602405][  T769] Read of size 832 at addr ffff888043ee6081 by task kworker/0:2/769
[   41.602898][  T769]
```

which led me on a different path. I assumed that out-of-bounds was occuring in
`mon_copy_to_buff` without realizing it may be the caller at fault, as Alan
pointed out in his feedback.

I now notice that my stacktrace is also slightly different (or rather
incomplete) as compared to the syzkaller report which says

```
BUG: KASAN: slab-out-of-bounds in mon_copy_to_buff drivers/usb/mon/mon_bin.c:252 [inline]
BUG: KASAN: slab-out-of-bounds in mon_bin_get_data drivers/usb/mon/mon_bin.c:420 [inline]
BUG: KASAN: slab-out-of-bounds in mon_bin_event+0x1211/0x2250 drivers/usb/mon/mon_bin.c:606
Read of size 832 at addr ffff88802888f1e1 by task kworker/0:2/979
```

where it does mention that the issue is in the caller. The caller must ensure
the correctness of write buffer.

Also, Hillf has produced a patch [2] which looks better than mine.


[1] https://syzkaller.appspot.com/text?tag=ReproC&x=1770d770580000
[2] https://lore.kernel.org/all/20250703043448.2287-1-hdanton@sina.com/

-- 
Manas

Re: [PATCH] usbmon: Fix out-of-bounds read in mon_copy_to_buff

[Alan Stern]

On Thu, Jul 03, 2025 at 11:10:14AM +0530, Manas wrote:
> On 03.07.2025 07:12, Greg Kroah-Hartman wrote:
> > On Thu, Jul 03, 2025 at 02:57:40AM +0530, Manas Gupta via B4 Relay wrote:
> > > From: Manas Gupta <manas18244@iiitd.ac.in>
> > > 
> > > memcpy tries to copy buffer 'from' when it is empty. This leads to
> > > out-of-bounds crash.
> > 
> > What exactly is the "crash" that you are seeing?  What is reporting it,
> > and how?
> > 
> Hi Greg and Alan,
> 
> I ran the reproducer[1] on my machine and got the following stacktrace.
> 
> ```
> [   41.601410][  T769] ==================================================================
> [   41.601908][  T769] BUG: KASAN: slab-out-of-bounds in mon_copy_to_buff+0xc6/0x180
> [   41.602405][  T769] Read of size 832 at addr ffff888043ee6081 by task kworker/0:2/769
> [   41.602898][  T769]
> ```
> 
> which led me on a different path. I assumed that out-of-bounds was occuring in
> `mon_copy_to_buff` without realizing it may be the caller at fault, as Alan
> pointed out in his feedback.
> 
> I now notice that my stacktrace is also slightly different (or rather
> incomplete) as compared to the syzkaller report which says
> 
> ```
> BUG: KASAN: slab-out-of-bounds in mon_copy_to_buff drivers/usb/mon/mon_bin.c:252 [inline]
> BUG: KASAN: slab-out-of-bounds in mon_bin_get_data drivers/usb/mon/mon_bin.c:420 [inline]
> BUG: KASAN: slab-out-of-bounds in mon_bin_event+0x1211/0x2250 drivers/usb/mon/mon_bin.c:606
> Read of size 832 at addr ffff88802888f1e1 by task kworker/0:2/979
> ```
> 
> where it does mention that the issue is in the caller. The caller must ensure
> the correctness of write buffer.
> 
> Also, Hillf has produced a patch [2] which looks better than mine.
> 
> 
> [1] https://syzkaller.appspot.com/text?tag=ReproC&x=1770d770580000
> [2] https://lore.kernel.org/all/20250703043448.2287-1-hdanton@sina.com/

Hillf's patch isn't right either.  The problem is that 
mon_copy_to_buff() is being called with invalid arguments.  The proper 
fix is not to check for bad arguments but rather to adjust the code that 
computes the bad argument, which in this case is either mon_bin_event() 
or mon_bin_get_data().

mon_bin_event() goes through a fairly long preparation, and it may 
calculate length incorrectly, although at the moment I don't see how.  
mon_bin_get_data() seems to be pretty straightforward.

I would start by figuring out which pathway is involved in the error and 
logging the incorrect values to see where they come from.

Alan Stern