| Message ID | tencent_088B2EF2AEE00C8AE7D706CCD2CBC6484906@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | USB: usbtmc: prevent kernel-infoleak | expand |
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
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
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
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
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
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;
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(+)