| Message ID | tencent_5F7E6CD82F3D6FB49414E9D6EC3ACCFC780A@qq.com (mailing list archive) |
|---|---|
| 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
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@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. 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. BR, Edward
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@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. > 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. thanks, greg k-h
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@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. > > 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@qq.com BR, Edward
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@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. > > > 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@qq.com 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
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(+)