* Re: [syzbot] [usb?] KMSAN: kernel-usb-infoleak in usbtmc_write
2024-09-04 12:00 ` syzbot
@ 2024-09-04 12:47 ` Edward Adam Davis
2024-09-04 13:33 ` syzbot
2024-09-04 13:55 ` [PATCH] USB: usbtmc: prevent kernel-infoleak Edward Adam Davis
` (6 subsequent siblings)
7 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-04 12:47 UTC (permalink / raw)
To: syzbot+9d34f80f841e948c3fdb; +Cc: linux-kernel, syzkaller-bugs
prevent kernel-infoleak.
We need to clear the structure before filling fields.
#syz test
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 6bd9fe565385..e9ddaa9b580d 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -759,6 +759,7 @@ static struct urb *usbtmc_create_urb(void)
usb_free_urb(urb);
return NULL;
}
+ memset(dmabuf, 0, bufsize);
urb->transfer_buffer = dmabuf;
urb->transfer_buffer_length = bufsize;
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH] USB: usbtmc: prevent kernel-infoleak
2024-09-04 12:00 ` syzbot
2024-09-04 12:47 ` Edward Adam Davis
@ 2024-09-04 13:55 ` Edward Adam Davis
2024-09-04 14:09 ` Greg KH
2024-09-05 11:27 ` [syzbot] [usb?] KMSAN: kernel-usb-infoleak in usbtmc_write Edward Adam Davis
` (5 subsequent siblings)
7 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-04 13:55 UTC (permalink / raw)
To: syzbot+9d34f80f841e948c3fdb
Cc: gregkh, linux-kernel, linux-usb, syzkaller-bugs
The syzbot reported a kernel-usb-infoleak in usbtmc_write,
we need to clear the structure before filling fields.
Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
drivers/usb/class/usbtmc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 6bd9fe565385..e9ddaa9b580d 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -759,6 +759,7 @@ static struct urb *usbtmc_create_urb(void)
usb_free_urb(urb);
return NULL;
}
+ memset(dmabuf, 0, bufsize);
urb->transfer_buffer = dmabuf;
urb->transfer_buffer_length = bufsize;
--
2.43.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH] USB: usbtmc: prevent kernel-infoleak
2024-09-04 13:55 ` [PATCH] USB: usbtmc: prevent kernel-infoleak Edward Adam Davis
@ 2024-09-04 14:09 ` Greg KH
2024-09-04 14:13 ` Greg KH
0 siblings, 1 reply; 40+ messages in thread
From: Greg KH @ 2024-09-04 14:09 UTC (permalink / raw)
To: Edward Adam Davis
Cc: syzbot+9d34f80f841e948c3fdb, linux-kernel, linux-usb,
syzkaller-bugs
On Wed, Sep 04, 2024 at 09:55:43PM +0800, Edward Adam Davis wrote:
> The syzbot reported a kernel-usb-infoleak in usbtmc_write,
> we need to clear the structure before filling fields.
Really?
>
> Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
> drivers/usb/class/usbtmc.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> index 6bd9fe565385..e9ddaa9b580d 100644
> --- a/drivers/usb/class/usbtmc.c
> +++ b/drivers/usb/class/usbtmc.c
> @@ -759,6 +759,7 @@ static struct urb *usbtmc_create_urb(void)
> usb_free_urb(urb);
> return NULL;
> }
> + memset(dmabuf, 0, bufsize);
To do this simpler, kzmalloc() above this would be nice.
But, this feels odd, where is the data leaking from? This is used for
both the read and write path, but where is the leak happening? A short
read? If so, we need to properly truncate the buffer being sent to
userspace and not send the unread data. If a short write, that makes no
sense.
So this needs a bit more work, please find the real issue here and don't
paper over the problem with "set it all to 0" like this.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH] USB: usbtmc: prevent kernel-infoleak
2024-09-04 14:09 ` Greg KH
@ 2024-09-04 14:13 ` Greg KH
2024-09-05 13:56 ` Edward Adam Davis
0 siblings, 1 reply; 40+ messages in thread
From: Greg KH @ 2024-09-04 14:13 UTC (permalink / raw)
To: Edward Adam Davis
Cc: syzbot+9d34f80f841e948c3fdb, linux-kernel, linux-usb,
syzkaller-bugs
On Wed, Sep 04, 2024 at 04:09:15PM +0200, Greg KH wrote:
> On Wed, Sep 04, 2024 at 09:55:43PM +0800, Edward Adam Davis wrote:
> > The syzbot reported a kernel-usb-infoleak in usbtmc_write,
> > we need to clear the structure before filling fields.
>
> Really?
>
>
> >
> > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > ---
> > drivers/usb/class/usbtmc.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > index 6bd9fe565385..e9ddaa9b580d 100644
> > --- a/drivers/usb/class/usbtmc.c
> > +++ b/drivers/usb/class/usbtmc.c
> > @@ -759,6 +759,7 @@ static struct urb *usbtmc_create_urb(void)
> > usb_free_urb(urb);
> > return NULL;
> > }
> > + memset(dmabuf, 0, bufsize);
>
> To do this simpler, kzmalloc() above this would be nice.
>
> But, this feels odd, where is the data leaking from? This is used for
> both the read and write path, but where is the leak happening? A short
> read? If so, we need to properly truncate the buffer being sent to
> userspace and not send the unread data. If a short write, that makes no
> sense.
I looked at the report and this seems to be data sent to the device, so
somehow we aren't setting the length to send to the device correctly.
Good luck!
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH] USB: usbtmc: prevent kernel-infoleak
2024-09-04 14:13 ` Greg KH
@ 2024-09-05 13:56 ` Edward Adam Davis
2024-09-05 14:04 ` Greg KH
0 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-05 13:56 UTC (permalink / raw)
To: gregkh
Cc: eadavis, linux-kernel, linux-usb, syzbot+9d34f80f841e948c3fdb,
syzkaller-bugs
On Wed, 4 Sep 2024 16:13:03 +0200, Greg KH wrote:
> On Wed, Sep 04, 2024 at 04:09:15PM +0200, Greg KH wrote:
> > On Wed, Sep 04, 2024 at 09:55:43PM +0800, Edward Adam Davis wrote:
> > > The syzbot reported a kernel-usb-infoleak in usbtmc_write,
> > > we need to clear the structure before filling fields.
> >
> > Really?
> >
> >
> > >
> > > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > ---
> > > drivers/usb/class/usbtmc.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > > index 6bd9fe565385..e9ddaa9b580d 100644
> > > --- a/drivers/usb/class/usbtmc.c
> > > +++ b/drivers/usb/class/usbtmc.c
> > > @@ -759,6 +759,7 @@ static struct urb *usbtmc_create_urb(void)
> > > usb_free_urb(urb);
> > > return NULL;
> > > }
> > > + memset(dmabuf, 0, bufsize);
> >
> > To do this simpler, kzmalloc() above this would be nice.
> >
> > But, this feels odd, where is the data leaking from? This is used for
> > both the read and write path, but where is the leak happening? A short
> > read? If so, we need to properly truncate the buffer being sent to
> > userspace and not send the unread data. If a short write, that makes no
> > sense.
A short write.
>
> I looked at the report and this seems to be data sent to the device, so
> somehow we aren't setting the length to send to the device correctly.
The length of the data passed in by the user is 3 bytes, plus a TMC header
length of 12 bytes and an additional 3 bytes. The actual length of the
data sent to the device is 16 bytes((3 + 12 + 3)~3 = 16).
Normally, when executing copy_from_user, the 3 bytes data passed in by
the user is written after the TMC header, which initializes the first 15
bytes of the 16 bytes. But it is not yet clear why the 15th byte is not
initialized. The kernel data leaked to user space reported by Syzbot
should be it.
>
> Good luck!
BR,
Edward
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH] USB: usbtmc: prevent kernel-infoleak
2024-09-05 13:56 ` Edward Adam Davis
@ 2024-09-05 14:04 ` Greg KH
2024-09-05 14:16 ` Edward Adam Davis
0 siblings, 1 reply; 40+ messages in thread
From: Greg KH @ 2024-09-05 14:04 UTC (permalink / raw)
To: Edward Adam Davis
Cc: linux-kernel, linux-usb, syzbot+9d34f80f841e948c3fdb,
syzkaller-bugs
On Thu, Sep 05, 2024 at 09:56:53PM +0800, Edward Adam Davis wrote:
> On Wed, 4 Sep 2024 16:13:03 +0200, Greg KH wrote:
> > On Wed, Sep 04, 2024 at 04:09:15PM +0200, Greg KH wrote:
> > > On Wed, Sep 04, 2024 at 09:55:43PM +0800, Edward Adam Davis wrote:
> > > > The syzbot reported a kernel-usb-infoleak in usbtmc_write,
> > > > we need to clear the structure before filling fields.
> > >
> > > Really?
> > >
> > >
> > > >
> > > > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > > > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > > > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > ---
> > > > drivers/usb/class/usbtmc.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > > > index 6bd9fe565385..e9ddaa9b580d 100644
> > > > --- a/drivers/usb/class/usbtmc.c
> > > > +++ b/drivers/usb/class/usbtmc.c
> > > > @@ -759,6 +759,7 @@ static struct urb *usbtmc_create_urb(void)
> > > > usb_free_urb(urb);
> > > > return NULL;
> > > > }
> > > > + memset(dmabuf, 0, bufsize);
> > >
> > > To do this simpler, kzmalloc() above this would be nice.
> > >
> > > But, this feels odd, where is the data leaking from? This is used for
> > > both the read and write path, but where is the leak happening? A short
> > > read? If so, we need to properly truncate the buffer being sent to
> > > userspace and not send the unread data. If a short write, that makes no
> > > sense.
> A short write.
> >
> > I looked at the report and this seems to be data sent to the device, so
> > somehow we aren't setting the length to send to the device correctly.
> The length of the data passed in by the user is 3 bytes, plus a TMC header
> length of 12 bytes and an additional 3 bytes. The actual length of the
> data sent to the device is 16 bytes((3 + 12 + 3)~3 = 16).
>
> Normally, when executing copy_from_user, the 3 bytes data passed in by
> the user is written after the TMC header, which initializes the first 15
> bytes of the 16 bytes. But it is not yet clear why the 15th byte is not
> initialized. The kernel data leaked to user space reported by Syzbot
> should be it.
But why are we sending 16 bytes to the device? Is that the format of
the message it expects? If so, that's fine, just set that byte to 0.
And as the device is the thing that is getting the kernel memory, that
really isn't a big deal as we "trust" hardware once it is up and talking
to the kernel.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH] USB: usbtmc: prevent kernel-infoleak
2024-09-05 14:04 ` Greg KH
@ 2024-09-05 14:16 ` Edward Adam Davis
2024-09-06 14:11 ` [PATCH V2] USB: usbtmc: prevent kernel-usb-infoleak Edward Adam Davis
0 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-05 14:16 UTC (permalink / raw)
To: gregkh
Cc: eadavis, linux-kernel, linux-usb, syzbot+9d34f80f841e948c3fdb,
syzkaller-bugs
On Thu, 5 Sep 2024 16:04:31 +0200, Greg KH wrote:
> On Thu, Sep 05, 2024 at 09:56:53PM +0800, Edward Adam Davis wrote:
> > On Wed, 4 Sep 2024 16:13:03 +0200, Greg KH wrote:
> > > On Wed, Sep 04, 2024 at 04:09:15PM +0200, Greg KH wrote:
> > > > On Wed, Sep 04, 2024 at 09:55:43PM +0800, Edward Adam Davis wrote:
> > > > > The syzbot reported a kernel-usb-infoleak in usbtmc_write,
> > > > > we need to clear the structure before filling fields.
> > > >
> > > > Really?
> > > >
> > > >
> > > > >
> > > > > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > > > > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > > > > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > > ---
> > > > > drivers/usb/class/usbtmc.c | 1 +
> > > > > 1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > > > > index 6bd9fe565385..e9ddaa9b580d 100644
> > > > > --- a/drivers/usb/class/usbtmc.c
> > > > > +++ b/drivers/usb/class/usbtmc.c
> > > > > @@ -759,6 +759,7 @@ static struct urb *usbtmc_create_urb(void)
> > > > > usb_free_urb(urb);
> > > > > return NULL;
> > > > > }
> > > > > + memset(dmabuf, 0, bufsize);
> > > >
> > > > To do this simpler, kzmalloc() above this would be nice.
> > > >
> > > > But, this feels odd, where is the data leaking from? This is used for
> > > > both the read and write path, but where is the leak happening? A short
> > > > read? If so, we need to properly truncate the buffer being sent to
> > > > userspace and not send the unread data. If a short write, that makes no
> > > > sense.
> > A short write.
> > >
> > > I looked at the report and this seems to be data sent to the device, so
> > > somehow we aren't setting the length to send to the device correctly.
> > The length of the data passed in by the user is 3 bytes, plus a TMC header
> > length of 12 bytes and an additional 3 bytes. The actual length of the
> > data sent to the device is 16 bytes((3 + 12 + 3)~3 = 16).
> >
> > Normally, when executing copy_from_user, the 3 bytes data passed in by
> > the user is written after the TMC header, which initializes the first 15
> > bytes of the 16 bytes. But it is not yet clear why the 15th byte is not
> > initialized. The kernel data leaked to user space reported by Syzbot
> > should be it.
>
> But why are we sending 16 bytes to the device? Is that the format of
> the message it expects? If so, that's fine, just set that byte to 0.
Yes. I have set them to 0 before calling copy_from_user, and now still
running tests.
https://syzkaller.appspot.com/text?tag=Patch&x=14cb8f33980000
>
> And as the device is the thing that is getting the kernel memory, that
> really isn't a big deal as we "trust" hardware once it is up and talking
> to the kernel.
:)
BR,
Edward
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH V2] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-05 14:16 ` Edward Adam Davis
@ 2024-09-06 14:11 ` Edward Adam Davis
2024-09-06 14:28 ` Alan Stern
0 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-06 14:11 UTC (permalink / raw)
To: eadavis
Cc: gregkh, linux-kernel, linux-usb, syzbot+9d34f80f841e948c3fdb,
syzkaller-bugs
The syzbot reported a kernel-usb-infoleak in usbtmc_write.
The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
in usbtmcw_write() follows the following pattern:
aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
aligned = (9 + 12 + 3) & ~3 = 24
...
Note: #define USBTMC_HEADER_SIZE 12
This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
subsequent memory not being initialized.
The condition aligned < buflen is used to avoid out of bounds access to
the buffer[USBTMC_HEADER_SIZE + transfersize] when "transfersize =
buflen - USBTMC_HEADER_SIZE".
Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
drivers/usb/class/usbtmc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 6bd9fe565385..faf8c5508997 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
goto exit;
}
+ if (aligned < buflen && (transfersize % 4))
+ memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
+ aligned - USBTMC_HEADER_SIZE - transfersize);
+
dev_dbg(&data->intf->dev, "%s(size:%u align:%u)\n", __func__,
(unsigned int)transfersize, (unsigned int)aligned);
--
2.43.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH V2] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-06 14:11 ` [PATCH V2] USB: usbtmc: prevent kernel-usb-infoleak Edward Adam Davis
@ 2024-09-06 14:28 ` Alan Stern
2024-09-07 2:08 ` Edward Adam Davis
0 siblings, 1 reply; 40+ messages in thread
From: Alan Stern @ 2024-09-06 14:28 UTC (permalink / raw)
To: Edward Adam Davis
Cc: gregkh, linux-kernel, linux-usb, syzbot+9d34f80f841e948c3fdb,
syzkaller-bugs
On Fri, Sep 06, 2024 at 10:11:03PM +0800, Edward Adam Davis wrote:
> The syzbot reported a kernel-usb-infoleak in usbtmc_write.
>
> The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
> in usbtmcw_write() follows the following pattern:
>
> aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
> aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
> aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
> aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
> aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
> aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
> aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
> aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
> aligned = (9 + 12 + 3) & ~3 = 24
> ...
What is the purpose of aligned? Why doesn't the driver simply use
USBTMC_HEADER_SIZE + transfersize instead of rounding it up to a
multiple of 4?
> Note: #define USBTMC_HEADER_SIZE 12
>
> This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
> subsequent memory not being initialized.
>
> The condition aligned < buflen is used to avoid out of bounds access to
> the buffer[USBTMC_HEADER_SIZE + transfersize] when "transfersize =
> buflen - USBTMC_HEADER_SIZE".
>
> Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
> drivers/usb/class/usbtmc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> index 6bd9fe565385..faf8c5508997 100644
> --- a/drivers/usb/class/usbtmc.c
> +++ b/drivers/usb/class/usbtmc.c
> @@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> goto exit;
> }
>
> + if (aligned < buflen && (transfersize % 4))
Shouldn't this be
if (USBTMC_HEADER_SIZE + transfersize < aligned)
?
Alan Stern
> + memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
> + aligned - USBTMC_HEADER_SIZE - transfersize);
> +
> dev_dbg(&data->intf->dev, "%s(size:%u align:%u)\n", __func__,
> (unsigned int)transfersize, (unsigned int)aligned);
>
> --
> 2.43.0
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V2] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-06 14:28 ` Alan Stern
@ 2024-09-07 2:08 ` Edward Adam Davis
2024-09-07 14:45 ` Alan Stern
0 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-07 2:08 UTC (permalink / raw)
To: stern
Cc: eadavis, gregkh, linux-kernel, linux-usb,
syzbot+9d34f80f841e948c3fdb, syzkaller-bugs
On Fri, 6 Sep 2024 10:28:11 -0400, Alan Stern wrote:
> On Fri, Sep 06, 2024 at 10:11:03PM +0800, Edward Adam Davis wrote:
> > The syzbot reported a kernel-usb-infoleak in usbtmc_write.
> >
> > The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
> > in usbtmcw_write() follows the following pattern:
> >
> > aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
> > aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
> > aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
> > aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
> > aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
> > aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
> > aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
> > aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
> > aligned = (9 + 12 + 3) & ~3 = 24
> > ...
>
> What is the purpose of aligned? Why doesn't the driver simply use
> USBTMC_HEADER_SIZE + transfersize instead of rounding it up to a
> multiple of 4?
I just found out that the logic of aligned calculation is like this.
As for why it is calculated like this, perhaps Guido Kiener can provide
a clearer explanation.
It was introduced by commit 4d5e18d9ed93 ("usb: usbtmc: Optimize usbtmc_write").
>
> > Note: #define USBTMC_HEADER_SIZE 12
> >
> > This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
> > subsequent memory not being initialized.
> >
> > The condition aligned < buflen is used to avoid out of bounds access to
> > the buffer[USBTMC_HEADER_SIZE + transfersize] when "transfersize =
> > buflen - USBTMC_HEADER_SIZE".
> >
> > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > ---
> > drivers/usb/class/usbtmc.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > index 6bd9fe565385..faf8c5508997 100644
> > --- a/drivers/usb/class/usbtmc.c
> > +++ b/drivers/usb/class/usbtmc.c
> > @@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> > goto exit;
> > }
> >
> > + if (aligned < buflen && (transfersize % 4))
>
> Shouldn't this be
>
> if (USBTMC_HEADER_SIZE + transfersize < aligned)
Logically, it seems possible to write it this way.
>
> ?
>
> Alan Stern
>
> > + memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
> > + aligned - USBTMC_HEADER_SIZE - transfersize);
> > +
> > dev_dbg(&data->intf->dev, "%s(size:%u align:%u)\n", __func__,
> > (unsigned int)transfersize, (unsigned int)aligned);
BR,
Edward
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V2] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-07 2:08 ` Edward Adam Davis
@ 2024-09-07 14:45 ` Alan Stern
2024-09-08 0:59 ` Edward Adam Davis
0 siblings, 1 reply; 40+ messages in thread
From: Alan Stern @ 2024-09-07 14:45 UTC (permalink / raw)
To: Edward Adam Davis
Cc: gregkh, linux-kernel, linux-usb, syzbot+9d34f80f841e948c3fdb,
syzkaller-bugs
On Sat, Sep 07, 2024 at 10:08:57AM +0800, Edward Adam Davis wrote:
> On Fri, 6 Sep 2024 10:28:11 -0400, Alan Stern wrote:
> > On Fri, Sep 06, 2024 at 10:11:03PM +0800, Edward Adam Davis wrote:
> > > The syzbot reported a kernel-usb-infoleak in usbtmc_write.
> > >
> > > The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
> > > in usbtmcw_write() follows the following pattern:
> > >
> > > aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
> > > aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
> > > aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
> > > aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
> > > aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
> > > aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
> > > aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
> > > aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
> > > aligned = (9 + 12 + 3) & ~3 = 24
> > > ...
> >
> > What is the purpose of aligned? Why doesn't the driver simply use
> > USBTMC_HEADER_SIZE + transfersize instead of rounding it up to a
> > multiple of 4?
> I just found out that the logic of aligned calculation is like this.
> As for why it is calculated like this, perhaps Guido Kiener can provide
> a clearer explanation.
> It was introduced by commit 4d5e18d9ed93 ("usb: usbtmc: Optimize usbtmc_write").
> >
> > > Note: #define USBTMC_HEADER_SIZE 12
> > >
> > > This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
> > > subsequent memory not being initialized.
> > >
> > > The condition aligned < buflen is used to avoid out of bounds access to
> > > the buffer[USBTMC_HEADER_SIZE + transfersize] when "transfersize =
> > > buflen - USBTMC_HEADER_SIZE".
> > >
> > > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > ---
> > > drivers/usb/class/usbtmc.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > > index 6bd9fe565385..faf8c5508997 100644
> > > --- a/drivers/usb/class/usbtmc.c
> > > +++ b/drivers/usb/class/usbtmc.c
> > > @@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> > > goto exit;
> > > }
> > >
> > > + if (aligned < buflen && (transfersize % 4))
> >
> > Shouldn't this be
> >
> > if (USBTMC_HEADER_SIZE + transfersize < aligned)
> Logically, it seems possible to write it this way.
In fact, what you wrote is wrong. Consider the case where buflen is 32
and transfersize is 17. Then aligned = (12 + 17 + 3) & ~3 = 32, so your
condition would fail to initialize the extra 3 bytes.
Alan Stern
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V2] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-07 14:45 ` Alan Stern
@ 2024-09-08 0:59 ` Edward Adam Davis
2024-09-08 1:32 ` Alan Stern
0 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-08 0:59 UTC (permalink / raw)
To: stern
Cc: eadavis, gregkh, linux-kernel, linux-usb,
syzbot+9d34f80f841e948c3fdb, syzkaller-bugs
On Sat, 7 Sep 2024 10:45:52 -0400, Alan Stern wrote:
> On Sat, Sep 07, 2024 at 10:08:57AM +0800, Edward Adam Davis wrote:
> > On Fri, 6 Sep 2024 10:28:11 -0400, Alan Stern wrote:
> > > On Fri, Sep 06, 2024 at 10:11:03PM +0800, Edward Adam Davis wrote:
> > > > The syzbot reported a kernel-usb-infoleak in usbtmc_write.
> > > >
> > > > The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
> > > > in usbtmcw_write() follows the following pattern:
> > > >
> > > > aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
> > > > aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
> > > > aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
> > > > aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
> > > > aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
> > > > aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
> > > > aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
> > > > aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
> > > > aligned = (9 + 12 + 3) & ~3 = 24
> > > > ...
> > >
> > > What is the purpose of aligned? Why doesn't the driver simply use
> > > USBTMC_HEADER_SIZE + transfersize instead of rounding it up to a
> > > multiple of 4?
> > I just found out that the logic of aligned calculation is like this.
> > As for why it is calculated like this, perhaps Guido Kiener can provide
> > a clearer explanation.
> > It was introduced by commit 4d5e18d9ed93 ("usb: usbtmc: Optimize usbtmc_write").
> > >
> > > > Note: #define USBTMC_HEADER_SIZE 12
> > > >
> > > > This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
> > > > subsequent memory not being initialized.
> > > >
> > > > The condition aligned < buflen is used to avoid out of bounds access to
> > > > the buffer[USBTMC_HEADER_SIZE + transfersize] when "transfersize =
> > > > buflen - USBTMC_HEADER_SIZE".
> > > >
> > > > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > > > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > > > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > ---
> > > > drivers/usb/class/usbtmc.c | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > > > index 6bd9fe565385..faf8c5508997 100644
> > > > --- a/drivers/usb/class/usbtmc.c
> > > > +++ b/drivers/usb/class/usbtmc.c
> > > > @@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> > > > goto exit;
> > > > }
> > > >
> > > > + if (aligned < buflen && (transfersize % 4))
> > >
> > > Shouldn't this be
> > >
> > > if (USBTMC_HEADER_SIZE + transfersize < aligned)
> > Logically, it seems possible to write it this way.
>
> In fact, what you wrote is wrong. Consider the case where buflen is 32
> and transfersize is 17. Then aligned = (12 + 17 + 3) & ~3 = 32, so your
> condition would fail to initialize the extra 3 bytes.
The buflen is equal to USBTMC_BUFSIZE and can not equal to any other value.
You can find it in usbtmc_create_urb() and usbtmc_write().
Note: #define USBTMC_BUFSIZE (4096)
BR,
Edward
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V2] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-08 0:59 ` Edward Adam Davis
@ 2024-09-08 1:32 ` Alan Stern
2024-09-08 2:01 ` Edward Adam Davis
2024-09-08 2:20 ` [PATCH V3] " Edward Adam Davis
0 siblings, 2 replies; 40+ messages in thread
From: Alan Stern @ 2024-09-08 1:32 UTC (permalink / raw)
To: Edward Adam Davis
Cc: gregkh, linux-kernel, linux-usb, syzbot+9d34f80f841e948c3fdb,
syzkaller-bugs
On Sun, Sep 08, 2024 at 08:59:48AM +0800, Edward Adam Davis wrote:
> On Sat, 7 Sep 2024 10:45:52 -0400, Alan Stern wrote:
> > On Sat, Sep 07, 2024 at 10:08:57AM +0800, Edward Adam Davis wrote:
> > > On Fri, 6 Sep 2024 10:28:11 -0400, Alan Stern wrote:
> > > > On Fri, Sep 06, 2024 at 10:11:03PM +0800, Edward Adam Davis wrote:
> > > > > The syzbot reported a kernel-usb-infoleak in usbtmc_write.
> > > > >
> > > > > The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
> > > > > in usbtmcw_write() follows the following pattern:
> > > > >
> > > > > aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
> > > > > aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
> > > > > aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
> > > > > aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
> > > > > aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
> > > > > aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
> > > > > aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
> > > > > aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
> > > > > aligned = (9 + 12 + 3) & ~3 = 24
> > > > > ...
> > > >
> > > > What is the purpose of aligned? Why doesn't the driver simply use
> > > > USBTMC_HEADER_SIZE + transfersize instead of rounding it up to a
> > > > multiple of 4?
> > > I just found out that the logic of aligned calculation is like this.
> > > As for why it is calculated like this, perhaps Guido Kiener can provide
> > > a clearer explanation.
> > > It was introduced by commit 4d5e18d9ed93 ("usb: usbtmc: Optimize usbtmc_write").
> > > >
> > > > > Note: #define USBTMC_HEADER_SIZE 12
> > > > >
> > > > > This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
> > > > > subsequent memory not being initialized.
> > > > >
> > > > > The condition aligned < buflen is used to avoid out of bounds access to
> > > > > the buffer[USBTMC_HEADER_SIZE + transfersize] when "transfersize =
> > > > > buflen - USBTMC_HEADER_SIZE".
> > > > >
> > > > > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > > > > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > > > > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > > ---
> > > > > drivers/usb/class/usbtmc.c | 4 ++++
> > > > > 1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > > > > index 6bd9fe565385..faf8c5508997 100644
> > > > > --- a/drivers/usb/class/usbtmc.c
> > > > > +++ b/drivers/usb/class/usbtmc.c
> > > > > @@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> > > > > goto exit;
> > > > > }
> > > > >
> > > > > + if (aligned < buflen && (transfersize % 4))
> > > >
> > > > Shouldn't this be
> > > >
> > > > if (USBTMC_HEADER_SIZE + transfersize < aligned)
> > > Logically, it seems possible to write it this way.
> >
> > In fact, what you wrote is wrong. Consider the case where buflen is 32
> > and transfersize is 17. Then aligned = (12 + 17 + 3) & ~3 = 32, so your
> > condition would fail to initialize the extra 3 bytes.
> The buflen is equal to USBTMC_BUFSIZE and can not equal to any other value.
> You can find it in usbtmc_create_urb() and usbtmc_write().
>
> Note: #define USBTMC_BUFSIZE (4096)
All right, so what happens if transfersize is 4081? Then aligned will
be equal to 4096, so your condition would fail to initialize the extra 3
bytes.
Alan Stern
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V2] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-08 1:32 ` Alan Stern
@ 2024-09-08 2:01 ` Edward Adam Davis
2024-09-08 2:20 ` [PATCH V3] " Edward Adam Davis
1 sibling, 0 replies; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-08 2:01 UTC (permalink / raw)
To: stern
Cc: eadavis, gregkh, linux-kernel, linux-usb,
syzbot+9d34f80f841e948c3fdb, syzkaller-bugs
On Sat, 7 Sep 2024 21:32:28 -0400, Alan Stern wrote:
> On Sun, Sep 08, 2024 at 08:59:48AM +0800, Edward Adam Davis wrote:
> > On Sat, 7 Sep 2024 10:45:52 -0400, Alan Stern wrote:
> > > On Sat, Sep 07, 2024 at 10:08:57AM +0800, Edward Adam Davis wrote:
> > > > On Fri, 6 Sep 2024 10:28:11 -0400, Alan Stern wrote:
> > > > > On Fri, Sep 06, 2024 at 10:11:03PM +0800, Edward Adam Davis wrote:
> > > > > > The syzbot reported a kernel-usb-infoleak in usbtmc_write.
> > > > > >
> > > > > > The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
> > > > > > in usbtmcw_write() follows the following pattern:
> > > > > >
> > > > > > aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
> > > > > > aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
> > > > > > aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
> > > > > > aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
> > > > > > aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
> > > > > > aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
> > > > > > aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
> > > > > > aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
> > > > > > aligned = (9 + 12 + 3) & ~3 = 24
> > > > > > ...
> > > > >
> > > > > What is the purpose of aligned? Why doesn't the driver simply use
> > > > > USBTMC_HEADER_SIZE + transfersize instead of rounding it up to a
> > > > > multiple of 4?
> > > > I just found out that the logic of aligned calculation is like this.
> > > > As for why it is calculated like this, perhaps Guido Kiener can provide
> > > > a clearer explanation.
> > > > It was introduced by commit 4d5e18d9ed93 ("usb: usbtmc: Optimize usbtmc_write").
> > > > >
> > > > > > Note: #define USBTMC_HEADER_SIZE 12
> > > > > >
> > > > > > This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
> > > > > > subsequent memory not being initialized.
> > > > > >
> > > > > > The condition aligned < buflen is used to avoid out of bounds access to
> > > > > > the buffer[USBTMC_HEADER_SIZE + transfersize] when "transfersize =
> > > > > > buflen - USBTMC_HEADER_SIZE".
> > > > > >
> > > > > > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > > > > > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > > > > > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > > > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > > > ---
> > > > > > drivers/usb/class/usbtmc.c | 4 ++++
> > > > > > 1 file changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > > > > > index 6bd9fe565385..faf8c5508997 100644
> > > > > > --- a/drivers/usb/class/usbtmc.c
> > > > > > +++ b/drivers/usb/class/usbtmc.c
> > > > > > @@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> > > > > > goto exit;
> > > > > > }
> > > > > >
> > > > > > + if (aligned < buflen && (transfersize % 4))
> > > > >
> > > > > Shouldn't this be
> > > > >
> > > > > if (USBTMC_HEADER_SIZE + transfersize < aligned)
> > > > Logically, it seems possible to write it this way.
> > >
> > > In fact, what you wrote is wrong. Consider the case where buflen is 32
> > > and transfersize is 17. Then aligned = (12 + 17 + 3) & ~3 = 32, so your
> > > condition would fail to initialize the extra 3 bytes.
> > The buflen is equal to USBTMC_BUFSIZE and can not equal to any other value.
> > You can find it in usbtmc_create_urb() and usbtmc_write().
> >
> > Note: #define USBTMC_BUFSIZE (4096)
>
> All right, so what happens if transfersize is 4081? Then aligned will
> be equal to 4096, so your condition would fail to initialize the extra 3
> bytes.
Not bad, when the transfersize values are 4081, 4082, and 4083, the aligned
value is 4096, which will result in 1, 2, and 3 bytes not being initialized,
respectively. I will update my patch.
BR,
Edward
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH V3] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-08 1:32 ` Alan Stern
2024-09-08 2:01 ` Edward Adam Davis
@ 2024-09-08 2:20 ` Edward Adam Davis
2024-09-08 5:20 ` Greg KH
1 sibling, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-08 2:20 UTC (permalink / raw)
To: stern
Cc: eadavis, gregkh, linux-kernel, linux-usb,
syzbot+9d34f80f841e948c3fdb, syzkaller-bugs
The syzbot reported a kernel-usb-infoleak in usbtmc_write.
The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
in usbtmcw_write() follows the following pattern:
aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
aligned = (9 + 12 + 3) & ~3 = 24
...
Note: #define USBTMC_HEADER_SIZE 12
This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
subsequent memory not being initialized.
Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
V2 -> V3: Update condition and comments
drivers/usb/class/usbtmc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 6bd9fe565385..faf8c5508997 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
goto exit;
}
+ if (USBTMC_HEADER_SIZE + transfersize < aligned)
+ memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
+ aligned - USBTMC_HEADER_SIZE - transfersize);
+
dev_dbg(&data->intf->dev, "%s(size:%u align:%u)\n", __func__,
(unsigned int)transfersize, (unsigned int)aligned);
--
2.43.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH V3] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-08 2:20 ` [PATCH V3] " Edward Adam Davis
@ 2024-09-08 5:20 ` Greg KH
2024-09-08 7:35 ` Edward Adam Davis
0 siblings, 1 reply; 40+ messages in thread
From: Greg KH @ 2024-09-08 5:20 UTC (permalink / raw)
To: Edward Adam Davis
Cc: stern, linux-kernel, linux-usb, syzbot+9d34f80f841e948c3fdb,
syzkaller-bugs
On Sun, Sep 08, 2024 at 10:20:57AM +0800, Edward Adam Davis wrote:
> The syzbot reported a kernel-usb-infoleak in usbtmc_write.
>
> The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
> in usbtmcw_write() follows the following pattern:
>
> aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
> aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
> aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
> aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
> aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
> aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
> aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
> aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
> aligned = (9 + 12 + 3) & ~3 = 24
> ...
>
> Note: #define USBTMC_HEADER_SIZE 12
>
> This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
> subsequent memory not being initialized.
>
> Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
> V2 -> V3: Update condition and comments
>
> drivers/usb/class/usbtmc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> index 6bd9fe565385..faf8c5508997 100644
> --- a/drivers/usb/class/usbtmc.c
> +++ b/drivers/usb/class/usbtmc.c
> @@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> goto exit;
> }
>
> + if (USBTMC_HEADER_SIZE + transfersize < aligned)
> + memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
> + aligned - USBTMC_HEADER_SIZE - transfersize);
As this is now a pain to read/understand, and there's no comment
describing it so we'll not really understand it in a few months, let
alone years, how about we just do the trivial thing and make the
allocation with kzalloc() to start with? And put a comment there saying
why it's zeroed out.
Sorry, I thought this was going to be a lot simpler based on your first
patch than this type of logic.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V3] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-08 5:20 ` Greg KH
@ 2024-09-08 7:35 ` Edward Adam Davis
2024-09-08 7:54 ` Greg KH
0 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-08 7:35 UTC (permalink / raw)
To: gregkh
Cc: eadavis, linux-kernel, linux-usb, stern,
syzbot+9d34f80f841e948c3fdb, syzkaller-bugs
On Sun, 8 Sep 2024 07:20:40 +0200, Greg KH wrote:
> On Sun, Sep 08, 2024 at 10:20:57AM +0800, Edward Adam Davis wrote:
> > The syzbot reported a kernel-usb-infoleak in usbtmc_write.
> >
> > The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
> > in usbtmcw_write() follows the following pattern:
> >
> > aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
> > aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
> > aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
> > aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
> > aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
> > aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
> > aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
> > aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
> > aligned = (9 + 12 + 3) & ~3 = 24
> > ...
> >
> > Note: #define USBTMC_HEADER_SIZE 12
> >
> > This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
> > subsequent memory not being initialized.
> >
> > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > ---
> > V2 -> V3: Update condition and comments
> >
> > drivers/usb/class/usbtmc.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > index 6bd9fe565385..faf8c5508997 100644
> > --- a/drivers/usb/class/usbtmc.c
> > +++ b/drivers/usb/class/usbtmc.c
> > @@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> > goto exit;
> > }
> >
> > + if (USBTMC_HEADER_SIZE + transfersize < aligned)
> > + memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
> > + aligned - USBTMC_HEADER_SIZE - transfersize);
>
> As this is now a pain to read/understand, and there's no comment
> describing it so we'll not really understand it in a few months, let
> alone years, how about we just do the trivial thing and make the
> allocation with kzalloc() to start with? And put a comment there saying
> why it's zeroed out.
Perhaps I wrote too much in my comments, but in essence, the logic behind
this version's fix is:
When aligned is greater than (USBTMC_HEADER_SIZE+transfersize), there are
(aligned - (USBTMC_HEADER_SIZE+transfersize) bytes after the header and data
that have not been initialized, and these bytes are then set to 0.
>
> Sorry, I thought this was going to be a lot simpler based on your first
> patch than this type of logic.
As you mentioned in my first version patch, this approach is simple and
easy to understand, but it comes at the cost of losing the real issue,
and KMSAN will not find similar problems again in the future, which is
not conducive to making the program logic more robust.
BR,
Edward
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V3] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-08 7:35 ` Edward Adam Davis
@ 2024-09-08 7:54 ` Greg KH
2024-09-08 8:16 ` Edward Adam Davis
0 siblings, 1 reply; 40+ messages in thread
From: Greg KH @ 2024-09-08 7:54 UTC (permalink / raw)
To: Edward Adam Davis
Cc: linux-kernel, linux-usb, stern, syzbot+9d34f80f841e948c3fdb,
syzkaller-bugs
On Sun, Sep 08, 2024 at 03:35:49PM +0800, Edward Adam Davis wrote:
> On Sun, 8 Sep 2024 07:20:40 +0200, Greg KH wrote:
> > On Sun, Sep 08, 2024 at 10:20:57AM +0800, Edward Adam Davis wrote:
> > > The syzbot reported a kernel-usb-infoleak in usbtmc_write.
> > >
> > > The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
> > > in usbtmcw_write() follows the following pattern:
> > >
> > > aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
> > > aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
> > > aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
> > > aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
> > > aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
> > > aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
> > > aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
> > > aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
> > > aligned = (9 + 12 + 3) & ~3 = 24
> > > ...
> > >
> > > Note: #define USBTMC_HEADER_SIZE 12
> > >
> > > This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
> > > subsequent memory not being initialized.
> > >
> > > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > ---
> > > V2 -> V3: Update condition and comments
> > >
> > > drivers/usb/class/usbtmc.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > > index 6bd9fe565385..faf8c5508997 100644
> > > --- a/drivers/usb/class/usbtmc.c
> > > +++ b/drivers/usb/class/usbtmc.c
> > > @@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> > > goto exit;
> > > }
> > >
> > > + if (USBTMC_HEADER_SIZE + transfersize < aligned)
> > > + memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
> > > + aligned - USBTMC_HEADER_SIZE - transfersize);
> >
> > As this is now a pain to read/understand, and there's no comment
> > describing it so we'll not really understand it in a few months, let
> > alone years, how about we just do the trivial thing and make the
> > allocation with kzalloc() to start with? And put a comment there saying
> > why it's zeroed out.
> Perhaps I wrote too much in my comments, but in essence, the logic behind
> this version's fix is:
> When aligned is greater than (USBTMC_HEADER_SIZE+transfersize), there are
> (aligned - (USBTMC_HEADER_SIZE+transfersize) bytes after the header and data
> that have not been initialized, and these bytes are then set to 0.
> >
> > Sorry, I thought this was going to be a lot simpler based on your first
> > patch than this type of logic.
> As you mentioned in my first version patch, this approach is simple and
> easy to understand, but it comes at the cost of losing the real issue,
> and KMSAN will not find similar problems again in the future, which is
> not conducive to making the program logic more robust.
There will not be similar problems in the future as you are explicitly
setting everything to 0, so all should be fine :)
The real issue here is that the usbtmc logic of sending data is crazy,
and unique to it for various reasons that well all really don't
understand. Given the very small number of these devices in the world,
it's probably best left to the maintainers of it to handle any real
problems going forward, and just squash these types of fuzzing bugs now
with a heavy hammer to make them happy.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V3] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-08 7:54 ` Greg KH
@ 2024-09-08 8:16 ` Edward Adam Davis
2024-09-08 8:33 ` Greg KH
0 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-08 8:16 UTC (permalink / raw)
To: gregkh
Cc: eadavis, linux-kernel, linux-usb, stern,
syzbot+9d34f80f841e948c3fdb, syzkaller-bugs
On Sun, 8 Sep 2024 09:54:22 +0200, Greg KH wrote:
> On Sun, Sep 08, 2024 at 03:35:49PM +0800, Edward Adam Davis wrote:
> > On Sun, 8 Sep 2024 07:20:40 +0200, Greg KH wrote:
> > > On Sun, Sep 08, 2024 at 10:20:57AM +0800, Edward Adam Davis wrote:
> > > > The syzbot reported a kernel-usb-infoleak in usbtmc_write.
> > > >
> > > > The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
> > > > in usbtmcw_write() follows the following pattern:
> > > >
> > > > aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
> > > > aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
> > > > aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
> > > > aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
> > > > aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
> > > > aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
> > > > aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
> > > > aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
> > > > aligned = (9 + 12 + 3) & ~3 = 24
> > > > ...
> > > >
> > > > Note: #define USBTMC_HEADER_SIZE 12
> > > >
> > > > This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
> > > > subsequent memory not being initialized.
> > > >
> > > > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > > > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > > > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > ---
> > > > V2 -> V3: Update condition and comments
> > > >
> > > > drivers/usb/class/usbtmc.c | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > > > index 6bd9fe565385..faf8c5508997 100644
> > > > --- a/drivers/usb/class/usbtmc.c
> > > > +++ b/drivers/usb/class/usbtmc.c
> > > > @@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> > > > goto exit;
> > > > }
> > > >
> > > > + if (USBTMC_HEADER_SIZE + transfersize < aligned)
> > > > + memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
> > > > + aligned - USBTMC_HEADER_SIZE - transfersize);
> > >
> > > As this is now a pain to read/understand, and there's no comment
> > > describing it so we'll not really understand it in a few months, let
> > > alone years, how about we just do the trivial thing and make the
> > > allocation with kzalloc() to start with? And put a comment there saying
> > > why it's zeroed out.
> > Perhaps I wrote too much in my comments, but in essence, the logic behind
> > this version's fix is:
> > When aligned is greater than (USBTMC_HEADER_SIZE+transfersize), there are
> > (aligned - (USBTMC_HEADER_SIZE+transfersize) bytes after the header and data
> > that have not been initialized, and these bytes are then set to 0.
> > >
> > > Sorry, I thought this was going to be a lot simpler based on your first
> > > patch than this type of logic.
> > As you mentioned in my first version patch, this approach is simple and
> > easy to understand, but it comes at the cost of losing the real issue,
> > and KMSAN will not find similar problems again in the future, which is
> > not conducive to making the program logic more robust.
>
> There will not be similar problems in the future as you are explicitly
> setting everything to 0, so all should be fine :)
>
> The real issue here is that the usbtmc logic of sending data is crazy,
> and unique to it for various reasons that well all really don't
> understand. Given the very small number of these devices in the world,
> it's probably best left to the maintainers of it to handle any real
> problems going forward, and just squash these types of fuzzing bugs now
> with a heavy hammer to make them happy.
I reserve my opinion.
If you insist, you can use my first patch directly:
https://lore.kernel.org/all/tencent_088B2EF2AEE00C8AE7D706CCD2CBC6484906@qq.com
BR,
Edward
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V3] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-08 8:16 ` Edward Adam Davis
@ 2024-09-08 8:33 ` Greg KH
2024-09-08 9:17 ` [PATCH v4] " Edward Adam Davis
0 siblings, 1 reply; 40+ messages in thread
From: Greg KH @ 2024-09-08 8:33 UTC (permalink / raw)
To: Edward Adam Davis
Cc: linux-kernel, linux-usb, stern, syzbot+9d34f80f841e948c3fdb,
syzkaller-bugs
On Sun, Sep 08, 2024 at 04:16:39PM +0800, Edward Adam Davis wrote:
> On Sun, 8 Sep 2024 09:54:22 +0200, Greg KH wrote:
> > On Sun, Sep 08, 2024 at 03:35:49PM +0800, Edward Adam Davis wrote:
> > > On Sun, 8 Sep 2024 07:20:40 +0200, Greg KH wrote:
> > > > On Sun, Sep 08, 2024 at 10:20:57AM +0800, Edward Adam Davis wrote:
> > > > > The syzbot reported a kernel-usb-infoleak in usbtmc_write.
> > > > >
> > > > > The expression "aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;"
> > > > > in usbtmcw_write() follows the following pattern:
> > > > >
> > > > > aligned = (1 + 12 + 3) & ~3 = 16 // 3 bytes have not been initialized
> > > > > aligned = (2 + 12 + 3) & ~3 = 16 // 2 bytes have not been initialized
> > > > > aligned = (3 + 12 + 3) & ~3 = 16 // 1 byte has not been initialized
> > > > > aligned = (4 + 12 + 3) & ~3 = 16 // All bytes have been initialized
> > > > > aligned = (5 + 12 + 3) & ~3 = 20 // 3 bytes have not been initialized
> > > > > aligned = (6 + 12 + 3) & ~3 = 20 // 2 bytes have not been initialized
> > > > > aligned = (7 + 12 + 3) & ~3 = 20 // 1 byte has not been initialized
> > > > > aligned = (8 + 12 + 3) & ~3 = 20 // All bytes have been initialized
> > > > > aligned = (9 + 12 + 3) & ~3 = 24
> > > > > ...
> > > > >
> > > > > Note: #define USBTMC_HEADER_SIZE 12
> > > > >
> > > > > This results in the buffer[USBTMC_SEAD_SIZE+transfersize] and its
> > > > > subsequent memory not being initialized.
> > > > >
> > > > > Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
> > > > > Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
> > > > > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
> > > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > > ---
> > > > > V2 -> V3: Update condition and comments
> > > > >
> > > > > drivers/usb/class/usbtmc.c | 4 ++++
> > > > > 1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > > > > index 6bd9fe565385..faf8c5508997 100644
> > > > > --- a/drivers/usb/class/usbtmc.c
> > > > > +++ b/drivers/usb/class/usbtmc.c
> > > > > @@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> > > > > goto exit;
> > > > > }
> > > > >
> > > > > + if (USBTMC_HEADER_SIZE + transfersize < aligned)
> > > > > + memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
> > > > > + aligned - USBTMC_HEADER_SIZE - transfersize);
> > > >
> > > > As this is now a pain to read/understand, and there's no comment
> > > > describing it so we'll not really understand it in a few months, let
> > > > alone years, how about we just do the trivial thing and make the
> > > > allocation with kzalloc() to start with? And put a comment there saying
> > > > why it's zeroed out.
> > > Perhaps I wrote too much in my comments, but in essence, the logic behind
> > > this version's fix is:
> > > When aligned is greater than (USBTMC_HEADER_SIZE+transfersize), there are
> > > (aligned - (USBTMC_HEADER_SIZE+transfersize) bytes after the header and data
> > > that have not been initialized, and these bytes are then set to 0.
> > > >
> > > > Sorry, I thought this was going to be a lot simpler based on your first
> > > > patch than this type of logic.
> > > As you mentioned in my first version patch, this approach is simple and
> > > easy to understand, but it comes at the cost of losing the real issue,
> > > and KMSAN will not find similar problems again in the future, which is
> > > not conducive to making the program logic more robust.
> >
> > There will not be similar problems in the future as you are explicitly
> > setting everything to 0, so all should be fine :)
> >
> > The real issue here is that the usbtmc logic of sending data is crazy,
> > and unique to it for various reasons that well all really don't
> > understand. Given the very small number of these devices in the world,
> > it's probably best left to the maintainers of it to handle any real
> > problems going forward, and just squash these types of fuzzing bugs now
> > with a heavy hammer to make them happy.
> I reserve my opinion.
>
> If you insist, you can use my first patch directly:
> https://lore.kernel.org/all/tencent_088B2EF2AEE00C8AE7D706CCD2CBC6484906@qq.com
No, that should be 'kzalloc()' instead of alocating and calling
memset(), to save us the round-trip of someone coming afterward and
cleaning up this common pattern to be a single call.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v4] USB: usbtmc: prevent kernel-usb-infoleak
2024-09-08 8:33 ` Greg KH
@ 2024-09-08 9:17 ` Edward Adam Davis
0 siblings, 0 replies; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-08 9:17 UTC (permalink / raw)
To: gregkh
Cc: eadavis, linux-kernel, linux-usb, stern,
syzbot+9d34f80f841e948c3fdb, syzkaller-bugs
The syzbot reported a kernel-usb-infoleak in usbtmc_write,
we need to clear the structure before filling fields.
Fixes: 4ddc645f40e9 ("usb: usbtmc: Add ioctl for vendor specific write")
Reported-and-tested-by: syzbot+9d34f80f841e948c3fdb@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
V1 -> V2: Only clear uninitialized bytes after header and data
V2 -> V3: Update condition and comments
V3 -> V4: Use kzalloc to clear up dmabuf
drivers/usb/class/usbtmc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 619123e24c41..7866ea6dad37 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -754,7 +754,7 @@ static struct urb *usbtmc_create_urb(void)
if (!urb)
return NULL;
- dmabuf = kmalloc(bufsize, GFP_KERNEL);
+ dmabuf = kzalloc(bufsize, GFP_KERNEL);
if (!dmabuf) {
usb_free_urb(urb);
return NULL;
--
2.43.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [syzbot] [usb?] KMSAN: kernel-usb-infoleak in usbtmc_write
2024-09-04 12:00 ` syzbot
2024-09-04 12:47 ` Edward Adam Davis
2024-09-04 13:55 ` [PATCH] USB: usbtmc: prevent kernel-infoleak Edward Adam Davis
@ 2024-09-05 11:27 ` Edward Adam Davis
2024-09-05 15:42 ` syzbot
2024-09-05 14:21 ` Edward Adam Davis
` (4 subsequent siblings)
7 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-05 11:27 UTC (permalink / raw)
To: syzbot+9d34f80f841e948c3fdb; +Cc: linux-kernel, syzkaller-bugs
clear save user data buf before copying data from user.
#syz test
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index e9ddaa9b580d..19bfe25e675c 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -1586,6 +1586,7 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;
}
+ memset(&buffer[USBTMC_HEADER_SIZE], 0, aligned - count);
if (copy_from_user(&buffer[USBTMC_HEADER_SIZE], buf, transfersize)) {
retval = -EFAULT;
up(&file_data->limit_write_sem);
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [syzbot] [usb?] KMSAN: kernel-usb-infoleak in usbtmc_write
2024-09-04 12:00 ` syzbot
` (2 preceding siblings ...)
2024-09-05 11:27 ` [syzbot] [usb?] KMSAN: kernel-usb-infoleak in usbtmc_write Edward Adam Davis
@ 2024-09-05 14:21 ` Edward Adam Davis
2024-09-05 16:11 ` syzbot
2024-09-06 11:55 ` Edward Adam Davis
` (3 subsequent siblings)
7 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-05 14:21 UTC (permalink / raw)
To: syzbot+9d34f80f841e948c3fdb; +Cc: linux-kernel, syzkaller-bugs
clear save user data buf before copying data from user.
#syz test
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index e9ddaa9b580d..19bfe25e675c 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -1586,6 +1586,7 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
aligned = (transfersize + (USBTMC_HEADER_SIZE + 3)) & ~3;
}
+ memset(&buffer[USBTMC_HEADER_SIZE], 0, aligned - USBTMC_HEADER_SIZE);
if (copy_from_user(&buffer[USBTMC_HEADER_SIZE], buf, transfersize)) {
retval = -EFAULT;
up(&file_data->limit_write_sem);
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [syzbot] [usb?] KMSAN: kernel-usb-infoleak in usbtmc_write
2024-09-04 12:00 ` syzbot
` (3 preceding siblings ...)
2024-09-05 14:21 ` Edward Adam Davis
@ 2024-09-06 11:55 ` Edward Adam Davis
2024-09-06 12:29 ` syzbot
2024-09-06 12:37 ` Edward Adam Davis
` (2 subsequent siblings)
7 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-06 11:55 UTC (permalink / raw)
To: syzbot+9d34f80f841e948c3fdb; +Cc: linux-kernel, syzkaller-bugs
Clear the buffer that has not been set, after copying data from user space.
#syz test
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 6bd9fe565385..5159f1c01116 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -1590,6 +1590,8 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
up(&file_data->limit_write_sem);
goto exit;
}
+ memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
+ aligned - USBTMC_HEADER_SIZE - transfersize);
dev_dbg(&data->intf->dev, "%s(size:%u align:%u)\n", __func__,
(unsigned int)transfersize, (unsigned int)aligned);
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [syzbot] [usb?] KMSAN: kernel-usb-infoleak in usbtmc_write
2024-09-04 12:00 ` syzbot
` (4 preceding siblings ...)
2024-09-06 11:55 ` Edward Adam Davis
@ 2024-09-06 12:37 ` Edward Adam Davis
2024-09-06 13:07 ` syzbot
2024-09-06 13:06 ` Edward Adam Davis
2024-09-06 13:52 ` Edward Adam Davis
7 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-06 12:37 UTC (permalink / raw)
To: syzbot+9d34f80f841e948c3fdb; +Cc: linux-kernel, syzkaller-bugs
Clear the buffer that has not been set, after copying data from user space.
#syz test
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 6bd9fe565385..faf8c5508997 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
goto exit;
}
+ if (aligned < buflen)
+ memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
+ aligned - USBTMC_HEADER_SIZE - transfersize);
+
dev_dbg(&data->intf->dev, "%s(size:%u align:%u)\n", __func__,
(unsigned int)transfersize, (unsigned int)aligned);
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [syzbot] [usb?] KMSAN: kernel-usb-infoleak in usbtmc_write
2024-09-04 12:00 ` syzbot
` (5 preceding siblings ...)
2024-09-06 12:37 ` Edward Adam Davis
@ 2024-09-06 13:06 ` Edward Adam Davis
2024-09-06 13:51 ` syzbot
2024-09-06 13:52 ` Edward Adam Davis
7 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-06 13:06 UTC (permalink / raw)
To: syzbot+9d34f80f841e948c3fdb; +Cc: linux-kernel, syzkaller-bugs
Clear the buffer that has not been set, after copying data from user space.
#syz test
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 6bd9fe565385..faf8c5508997 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
goto exit;
}
+ if (transfersize <= 3)
+ memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
+ aligned - USBTMC_HEADER_SIZE - transfersize);
+
dev_dbg(&data->intf->dev, "%s(size:%u align:%u)\n", __func__,
(unsigned int)transfersize, (unsigned int)aligned);
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [syzbot] [usb?] KMSAN: kernel-usb-infoleak in usbtmc_write
2024-09-04 12:00 ` syzbot
` (6 preceding siblings ...)
2024-09-06 13:06 ` Edward Adam Davis
@ 2024-09-06 13:52 ` Edward Adam Davis
2024-09-06 16:59 ` syzbot
7 siblings, 1 reply; 40+ messages in thread
From: Edward Adam Davis @ 2024-09-06 13:52 UTC (permalink / raw)
To: syzbot+9d34f80f841e948c3fdb; +Cc: linux-kernel, syzkaller-bugs
Clear the buffer that has not been set, after copying data from user space.
#syz test
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 6bd9fe565385..faf8c5508997 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -1591,6 +1591,10 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
goto exit;
}
+ if (transfersize % 4)
+ memset(&buffer[USBTMC_HEADER_SIZE + transfersize], 0,
+ aligned - USBTMC_HEADER_SIZE - transfersize);
+
dev_dbg(&data->intf->dev, "%s(size:%u align:%u)\n", __func__,
(unsigned int)transfersize, (unsigned int)aligned);
^ permalink raw reply related [flat|nested] 40+ messages in thread