| Message ID | tencent_5F7E6CD82F3D6FB49414E9D6EC3ACCFC780A@qq.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V3] USB: usbtmc: prevent kernel-usb-infoleak | expand |
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@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > 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
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); + dev_dbg(&data->intf->dev, "%s(size:%u align:%u)\n", __func__, (unsigned int)transfersize, (unsigned int)aligned);
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@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=9d34f80f841e948c3fdb Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- V2 -> V3: Update condition and comments drivers/usb/class/usbtmc.c | 4 ++++ 1 file changed, 4 insertions(+)