drivers/usb/mon/mon_bin.c | 5 +++++ 1 file changed, 5 insertions(+)
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>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
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
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
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,
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,
>
© 2016 - 2025 Red Hat, Inc.