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@xxxxxxxxxxxxxxxxxxxxxxxxx > > > > > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb > > > > > Signed-off-by: Edward Adam Davis <eadavis@xxxxxx> > > > > > --- > > > > > 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