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. 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