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

[contact@arnaud-lcm.com]

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,