Re: [PATCH V3] USB: usbtmc: prevent kernel-usb-infoleak

[Greg KH]

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

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