[PATCH] usb: usbtmc: initialize memory written to device

[PATCH] usb: usbtmc: initialize memory written to device

[Pimyn]

Avoid kernel-usb-infoleak by initializing all memory written to device.
The buffer length uses 32bit alignment which might cause some buffer
data to be read without any initialization.

Reported-by: syzbot+8f282cce71948071c335@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/0000000000006f6622061eb52dba@google.com/T/
Signed-off-by: Pimyn Girgis <bemenboshra2001@gmail.com> (Google)
---
 drivers/usb/class/usbtmc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index ffc9c6fdd7e1..d79a08dfb54d 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -21,6 +21,7 @@
 #include <linux/usb.h>
 #include <linux/compat.h>
 #include <linux/usb/tmc.h>
+#include <linux/string.h>
 
 /* Increment API VERSION when changing tmc.h with new flags or ioctls
  * or when changing a significant behavior of the driver.
@@ -1169,6 +1170,8 @@ static ssize_t usbtmc_generic_write(struct usbtmc_file_data *file_data,
 		 * (size + 3 & ~3) rounds up and simplifies user code
 		 */
 		aligned = (this_part + 3) & ~3;
+		/* Initialize the remaining part of the buffer */
+		memzero_explicit(buffer + this_part, aligned - this_part);
 		dev_dbg(dev, "write(size:%u align:%u done:%u)\n",
 			(unsigned int)this_part,
 			(unsigned int)aligned,
-- 
2.47.0.rc0.187.ge670bccf7e-goog

Re: [PATCH] usb: usbtmc: initialize memory written to device

[Greg KH]

On Fri, Oct 04, 2024 at 12:33:13PM +0000, Pimyn@web.codeaurora.org wrote:
> Avoid kernel-usb-infoleak by initializing all memory written to device.
> The buffer length uses 32bit alignment which might cause some buffer
> data to be read without any initialization.
> 
> Reported-by: syzbot+8f282cce71948071c335@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/0000000000006f6622061eb52dba@google.com/T/
> Signed-off-by: Pimyn Girgis <bemenboshra2001@gmail.com> (Google)
> ---
>  drivers/usb/class/usbtmc.c | 3 +++
>  1 file changed, 3 insertions(+)

The "From:" line of this commit is obviously not correct and does not
match this signed-off-by line.

Also, as you are from google, just use your google.com email address
please.

thanks,

greg k-h

Re: [PATCH] usb: usbtmc: initialize memory written to device

[Alexander Potapenko]

On Fri, Oct 4, 2024 at 2:57 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Oct 04, 2024 at 12:33:13PM +0000, Pimyn@web.codeaurora.org wrote:

Just curious, what's up with this @web.codeaurora.org address? It
wasn't in the original patch, where did it come from? :)

> > Avoid kernel-usb-infoleak by initializing all memory written to device.
> > The buffer length uses 32bit alignment which might cause some buffer
> > data to be read without any initialization.
> >
> > Reported-by: syzbot+8f282cce71948071c335@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/all/0000000000006f6622061eb52dba@google.com/T/
> > Signed-off-by: Pimyn Girgis <bemenboshra2001@gmail.com> (Google)
> > ---
> >  drivers/usb/class/usbtmc.c | 3 +++
> >  1 file changed, 3 insertions(+)
>
> The "From:" line of this commit is obviously not correct and does not
> match this signed-off-by line.

Sorry, does it mean the "(Google)" part should be present in the From:
line as well?

> Also, as you are from google, just use your google.com email address
> please.

Unfortunately Pimyn cannot send emails outside the @google.com domain.

Re: [PATCH] usb: usbtmc: initialize memory written to device

[Greg KH]

On Fri, Oct 04, 2024 at 03:41:22PM +0200, Alexander Potapenko wrote:
> On Fri, Oct 4, 2024 at 2:57 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Oct 04, 2024 at 12:33:13PM +0000, Pimyn@web.codeaurora.org wrote:
> 
> Just curious, what's up with this @web.codeaurora.org address? It
> wasn't in the original patch, where did it come from? :)

You tell me, look at the original patch, it's in the email...

> > > Avoid kernel-usb-infoleak by initializing all memory written to device.
> > > The buffer length uses 32bit alignment which might cause some buffer
> > > data to be read without any initialization.
> > >
> > > Reported-by: syzbot+8f282cce71948071c335@syzkaller.appspotmail.com
> > > Closes: https://lore.kernel.org/all/0000000000006f6622061eb52dba@google.com/T/
> > > Signed-off-by: Pimyn Girgis <bemenboshra2001@gmail.com> (Google)
> > > ---
> > >  drivers/usb/class/usbtmc.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> >
> > The "From:" line of this commit is obviously not correct and does not
> > match this signed-off-by line.
> 
> Sorry, does it mean the "(Google)" part should be present in the From:
> line as well?

A valid email address should be in the "From:" line.

> > Also, as you are from google, just use your google.com email address
> > please.
> 
> Unfortunately Pimyn cannot send emails outside the @google.com domain.

Google has well-known facilities and abilities to handle this, please
use them and do not hide behind gmail accounts.

thanks,

greg k-h

Re: [PATCH] usb: usbtmc: initialize memory written to device

[Greg KH]

On Fri, Oct 04, 2024 at 12:33:13PM +0000, Pimyn@web.codeaurora.org wrote:
> Avoid kernel-usb-infoleak by initializing all memory written to device.
> The buffer length uses 32bit alignment which might cause some buffer
> data to be read without any initialization.
> 
> Reported-by: syzbot+8f282cce71948071c335@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/0000000000006f6622061eb52dba@google.com/T/
> Signed-off-by: Pimyn Girgis <bemenboshra2001@gmail.com> (Google)
> ---
>  drivers/usb/class/usbtmc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> index ffc9c6fdd7e1..d79a08dfb54d 100644
> --- a/drivers/usb/class/usbtmc.c
> +++ b/drivers/usb/class/usbtmc.c
> @@ -21,6 +21,7 @@
>  #include <linux/usb.h>
>  #include <linux/compat.h>
>  #include <linux/usb/tmc.h>
> +#include <linux/string.h>
>  
>  /* Increment API VERSION when changing tmc.h with new flags or ioctls
>   * or when changing a significant behavior of the driver.
> @@ -1169,6 +1170,8 @@ static ssize_t usbtmc_generic_write(struct usbtmc_file_data *file_data,
>  		 * (size + 3 & ~3) rounds up and simplifies user code
>  		 */
>  		aligned = (this_part + 3) & ~3;
> +		/* Initialize the remaining part of the buffer */
> +		memzero_explicit(buffer + this_part, aligned - this_part);

Why "explicit"?

Also, what about commit 625fa77151f0 ("USB: usbtmc: prevent
kernel-usb-infoleak"), doesn't that solve the issue here?

thanks,

greg k-h

Re: [PATCH] usb: usbtmc: initialize memory written to device

[Ⲡⲟⲓⲙⲏⲛ]

On Fri, 4 Oct 2024 at 14:58, Greg KH <gregkh@linuxfoundation.org> wrote:

> Why "explicit"?
Thank you. I think we could use memzero here instead.
> Also, what about commit 625fa77151f0 ("USB: usbtmc: prevent
> kernel-usb-infoleak"), doesn't that solve the issue here?
You're right. This commit is not in 6.11, so I missed it.