[PATCH] usb: mon: Fix slab-out-of-bounds in mon_bin_event due to unsafe URB transfer_buffer access

[PATCH] usb: mon: Fix slab-out-of-bounds in mon_bin_event due to unsafe URB transfer_buffer access

[Arnaud Lecomte]

The syzkaller fuzzer uncovered a kernel slab-out-of-bounds
 write in the USB monitoring subsystem (mon_bin) when handling
 a malformed URB (USB Request Block) with the following properties:
 - transfer_buffer_length = 0xffff
 - actual_length = 0x0 (no data transferred)
 - number_of_packets = 0x0 (non-isochronous transfer)

When reaching the mon_copy_to_buff function,
 we will try to copy into the mon rp bin with the following parameters:
off=0xcc0, from=0xffff8880246df5e1 "", length=0xf000

At the first iteration, the step_len is 0x340 and it is during the mem_cpy
that the slab-out-of-bounds happens.
As step_len < transfer_buffer_length, we can deduce that it is related
 to an issue with the transfer_buffer being invalid.
The patch proposes a safe access to the kernel
 kernel buffer urb->transfer_buffer with `copy_from_kernel_nofault`.

Reported-by: syzbot+86b6d7c8bcc66747c505@syzkaller.appspotmail.com
Fixes: 6f23ee1fefdc1 ("USB: add binary API to usbmon")
Closes: https://syzkaller.appspot.com/bug?extid=86b6d7c8bcc66747c505
Tested-by: syzbot+86b6d7c8bcc66747c505@syzkaller.appspotmail.com
Signed-off-by: Arnaud Lecomte <contact@arnaud-lcm.com>
---
 drivers/usb/mon/mon_bin.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index c93b43f5bc46..d3bef2a37eb0 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -249,7 +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;
-		memcpy(buf, from, step_len);
+
+		if (copy_from_kernel_nofault(buf, from, step_len)) {
+			pr_warn("Failed to copy URB transfer buffer content into mon bin.");
+			return -EFAULT;
+		}
 		if ((off += step_len) >= this->b_size) off = 0;
 		from += step_len;
 		length -= step_len;
@@ -413,11 +417,13 @@ static unsigned int mon_bin_get_data(const struct mon_reader_bin *rp,
 
 	*flag = 0;
 	if (urb->num_sgs == 0) {
-		if (urb->transfer_buffer == NULL) {
+		if (
+			urb->transfer_buffer == NULL ||
+			mon_copy_to_buff(rp, offset, urb->transfer_buffer, length) < 0
+		) {
 			*flag = 'Z';
 			return length;
 		}
-		mon_copy_to_buff(rp, offset, urb->transfer_buffer, length);
 		length = 0;
 
 	} else {
@@ -434,6 +440,10 @@ static unsigned int mon_bin_get_data(const struct mon_reader_bin *rp,
 			this_len = min_t(unsigned int, sg->length, length);
 			offset = mon_copy_to_buff(rp, offset, sg_virt(sg),
 					this_len);
+			if (offset < 0) {
+				*flag = 'Z';
+				return length;
+			}
 			length -= this_len;
 		}
 		if (i == 0)
-- 
2.43.0

Re: [PATCH] usb: mon: Fix slab-out-of-bounds in mon_bin_event due to unsafe URB transfer_buffer access

[Alan Stern]

On Sun, Jul 20, 2025 at 09:00:57PM +0100, Arnaud Lecomte wrote:
> The syzkaller fuzzer uncovered a kernel slab-out-of-bounds
>  write in the USB monitoring subsystem (mon_bin) when handling
>  a malformed URB (USB Request Block) with the following properties:
>  - transfer_buffer_length = 0xffff
>  - actual_length = 0x0 (no data transferred)
>  - number_of_packets = 0x0 (non-isochronous transfer)

This kind of problem is fixed not by changing the way mon_bin reacts to 
malformed URBs, but rather by correcting the code that creates the URBs 
in the first place so that they won't be malformed.

> When reaching the mon_copy_to_buff function,
>  we will try to copy into the mon rp bin with the following parameters:
> off=0xcc0, from=0xffff8880246df5e1 "", length=0xf000
> 
> At the first iteration, the step_len is 0x340 and it is during the mem_cpy
> that the slab-out-of-bounds happens.
> As step_len < transfer_buffer_length, we can deduce that it is related
>  to an issue with the transfer_buffer being invalid.
> The patch proposes a safe access to the kernel
>  kernel buffer urb->transfer_buffer with `copy_from_kernel_nofault`.
> 
> Reported-by: syzbot+86b6d7c8bcc66747c505@syzkaller.appspotmail.com
> Fixes: 6f23ee1fefdc1 ("USB: add binary API to usbmon")
> Closes: https://syzkaller.appspot.com/bug?extid=86b6d7c8bcc66747c505
> Tested-by: syzbot+86b6d7c8bcc66747c505@syzkaller.appspotmail.com
> Signed-off-by: Arnaud Lecomte <contact@arnaud-lcm.com>
> ---

This is unnecessary.  The underlying cause of the problem was fixed by 
commit 0d0777ccaa2d ("HID: core: ensure __hid_request reserves the 
report ID as the first byte") in the HID tree.

Alan Stern

Re: [PATCH] usb: mon: Fix slab-out-of-bounds in mon_bin_event due to unsafe URB transfer_buffer access

[Lecomte, Arnaud]

Hi Alan, thanks for your reply.

Your point raises an important question for me: Is there a specific 
reason why we don’t have
  a synchronization mechanism in place to protect the URB's transfer 
buffer ?


Arnaud


On 21/07/2025 02:29, Alan Stern wrote:
> On Sun, Jul 20, 2025 at 09:00:57PM +0100, Arnaud Lecomte wrote:
>> The syzkaller fuzzer uncovered a kernel slab-out-of-bounds
>>   write in the USB monitoring subsystem (mon_bin) when handling
>>   a malformed URB (USB Request Block) with the following properties:
>>   - transfer_buffer_length = 0xffff
>>   - actual_length = 0x0 (no data transferred)
>>   - number_of_packets = 0x0 (non-isochronous transfer)
> This kind of problem is fixed not by changing the way mon_bin reacts to
> malformed URBs, but rather by correcting the code that creates the URBs
> in the first place so that they won't be malformed.
>
>> When reaching the mon_copy_to_buff function,
>>   we will try to copy into the mon rp bin with the following parameters:
>> off=0xcc0, from=0xffff8880246df5e1 "", length=0xf000
>>
>> At the first iteration, the step_len is 0x340 and it is during the mem_cpy
>> that the slab-out-of-bounds happens.
>> As step_len < transfer_buffer_length, we can deduce that it is related
>>   to an issue with the transfer_buffer being invalid.
>> The patch proposes a safe access to the kernel
>>   kernel buffer urb->transfer_buffer with `copy_from_kernel_nofault`.
>>
>> Reported-by: syzbot+86b6d7c8bcc66747c505@syzkaller.appspotmail.com
>> Fixes: 6f23ee1fefdc1 ("USB: add binary API to usbmon")
>> Closes: https://syzkaller.appspot.com/bug?extid=86b6d7c8bcc66747c505
>> Tested-by: syzbot+86b6d7c8bcc66747c505@syzkaller.appspotmail.com
>> Signed-off-by: Arnaud Lecomte <contact@arnaud-lcm.com>
>> ---
> This is unnecessary.  The underlying cause of the problem was fixed by
> commit 0d0777ccaa2d ("HID: core: ensure __hid_request reserves the
> report ID as the first byte") in the HID tree.
>
> Alan Stern

Re: [PATCH] usb: mon: Fix slab-out-of-bounds in mon_bin_event due to unsafe URB transfer_buffer access

[Alan Stern]

On Mon, Jul 21, 2025 at 09:22:40AM +0100, Lecomte, Arnaud wrote:
> Hi Alan, thanks for your reply.
> 
> Your point raises an important question for me: Is there a specific reason
> why we don’t have
>  a synchronization mechanism in place to protect the URB's transfer buffer ?

Protect it from what?  Access by some driver at an inappropriate time?  
Drivers are supposed to know (and this is alluded to in the kerneldoc 
for usb_submit_urb()) that they aren't allowed to touch the transfer 
buffer while an URB is queued.

Alan Stern

Re: [PATCH] usb: mon: Fix slab-out-of-bounds in mon_bin_event due to unsafe URB transfer_buffer access

[Lecomte, Arnaud]

It clarifies things and makes more sense now.
Appreciate the explanation :), thanks for your time

Arnaud

On 21/07/2025 14:51, Alan Stern wrote:
> On Mon, Jul 21, 2025 at 09:22:40AM +0100, Lecomte, Arnaud wrote:
>> Hi Alan, thanks for your reply.
>>
>> Your point raises an important question for me: Is there a specific reason
>> why we don’t have
>>   a synchronization mechanism in place to protect the URB's transfer buffer ?
> Protect it from what?  Access by some driver at an inappropriate time?
> Drivers are supposed to know (and this is alluded to in the kerneldoc
> for usb_submit_urb()) that they aren't allowed to touch the transfer
> buffer while an URB is queued.
>
> Alan Stern

Re: [PATCH] usb: mon: Fix slab-out-of-bounds in mon_bin_event due to unsafe URB transfer_buffer access

[kernel test robot]

Hi Arnaud,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus westeri-thunderbolt/next linus/master v6.16-rc7 next-20250722]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Arnaud-Lecomte/usb-mon-Fix-slab-out-of-bounds-in-mon_bin_event-due-to-unsafe-URB-transfer_buffer-access/20250721-040222
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20250720200057.19720-1-contact%40arnaud-lcm.com
patch subject: [PATCH] usb: mon: Fix slab-out-of-bounds in mon_bin_event due to unsafe URB transfer_buffer access
config: m68k-randconfig-r073-20250723 (https://download.01.org/0day-ci/archive/20250723/202507230548.g6zwppI6-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.3.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507230548.g6zwppI6-lkp@intel.com/

smatch warnings:
drivers/usb/mon/mon_bin.c:422 mon_bin_get_data() warn: unsigned 'mon_copy_to_buff(rp, offset, urb->transfer_buffer, length)' is never less than zero.
drivers/usb/mon/mon_bin.c:443 mon_bin_get_data() warn: unsigned 'offset' is never less than zero.

vim +422 drivers/usb/mon/mon_bin.c

   409	
   410	static unsigned int mon_bin_get_data(const struct mon_reader_bin *rp,
   411	    unsigned int offset, struct urb *urb, unsigned int length,
   412	    char *flag)
   413	{
   414		int i;
   415		struct scatterlist *sg;
   416		unsigned int this_len;
   417	
   418		*flag = 0;
   419		if (urb->num_sgs == 0) {
   420			if (
   421				urb->transfer_buffer == NULL ||
 > 422				mon_copy_to_buff(rp, offset, urb->transfer_buffer, length) < 0
   423			) {
   424				*flag = 'Z';
   425				return length;
   426			}
   427			length = 0;
   428	
   429		} else {
   430			/* If IOMMU coalescing occurred, we cannot trust sg_page */
   431			if (urb->transfer_flags & URB_DMA_SG_COMBINED) {
   432				*flag = 'D';
   433				return length;
   434			}
   435	
   436			/* Copy up to the first non-addressable segment */
   437			for_each_sg(urb->sg, sg, urb->num_sgs, i) {
   438				if (length == 0 || PageHighMem(sg_page(sg)))
   439					break;
   440				this_len = min_t(unsigned int, sg->length, length);
   441				offset = mon_copy_to_buff(rp, offset, sg_virt(sg),
   442						this_len);
 > 443				if (offset < 0) {
   444					*flag = 'Z';
   445					return length;
   446				}
   447				length -= this_len;
   448			}
   449			if (i == 0)
   450				*flag = 'D';
   451		}
   452	
   453		return length;
   454	}
   455	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki