| Message ID | 20220106225716.7425-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d668769eb9c52b150753f1653f7f5a0aeb8239d2 |
| Headers | show |
| Series | net: mcs7830: handle usb read errors properly | expand |
On Thu, Jan 6, 2022 at 5:57 PM Pavel Skripkin <paskripkin@gmail.com> wrote: > > Syzbot reported uninit value in mcs7830_bind(). The problem was in > missing validation check for bytes read via usbnet_read_cmd(). > > usbnet_read_cmd() internally calls usb_control_msg(), that returns > number of bytes read. Code should validate that requested number of bytes > was actually read. > > So, this patch adds missing size validation check inside > mcs7830_get_reg() to prevent uninit value bugs > > CC: Arnd Bergmann <arnd@arndb.de> > Reported-and-tested-by: syzbot+003c0a286b9af5412510@syzkaller.appspotmail.com > Fixes: 2a36d7083438 ("USB: driver for mcs7830 (aka DeLOCK) USB ethernet adapter") Looks good to me. Reviewed-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > --- > > @Arnd, I am not sure about mcs7830_get_rev() function. > > Is get_reg(22, 2) == 1 valid read? If so, I think, we should call > usbnet_read_cmd() directly here, since other callers care only about > negative error values. I have no idea, I never had a datasheet for this device, only the hardware I bought cheaply and vendor source code I found somewhere on the net, and that was 16 years ago. I would not expect the hardware to ever return less data than was asked for, so any length checking would only have to account for attackers that fake this device. Arnd
On Fri, Jan 07, 2022 at 01:57:16AM +0300, Pavel Skripkin wrote: > Syzbot reported uninit value in mcs7830_bind(). The problem was in > missing validation check for bytes read via usbnet_read_cmd(). > > usbnet_read_cmd() internally calls usb_control_msg(), that returns > number of bytes read. Code should validate that requested number of bytes > was actually read. > > So, this patch adds missing size validation check inside > mcs7830_get_reg() to prevent uninit value bugs > > CC: Arnd Bergmann <arnd@arndb.de> > Reported-and-tested-by: syzbot+003c0a286b9af5412510@syzkaller.appspotmail.com > Fixes: 2a36d7083438 ("USB: driver for mcs7830 (aka DeLOCK) USB ethernet adapter") > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > --- > > @Arnd, I am not sure about mcs7830_get_rev() function. > > Is get_reg(22, 2) == 1 valid read? If so, I think, we should call > usbnet_read_cmd() directly here, since other callers care only about > negative error values. > > Thanks > > > --- > drivers/net/usb/mcs7830.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/usb/mcs7830.c b/drivers/net/usb/mcs7830.c > index 326cc4e749d8..fdda0616704e 100644 > --- a/drivers/net/usb/mcs7830.c > +++ b/drivers/net/usb/mcs7830.c > @@ -108,8 +108,16 @@ static const char driver_name[] = "MOSCHIP usb-ethernet driver"; > > static int mcs7830_get_reg(struct usbnet *dev, u16 index, u16 size, void *data) > { > - return usbnet_read_cmd(dev, MCS7830_RD_BREQ, MCS7830_RD_BMREQ, > - 0x0000, index, data, size); > + int ret; > + > + ret = usbnet_read_cmd(dev, MCS7830_RD_BREQ, MCS7830_RD_BMREQ, > + 0x0000, index, data, size); > + if (ret < 0) > + return ret; > + else if (ret < size) > + return -ENODATA; We have a usb core function that handles these "short reads are an error" issue. Perhaps usbnet_read_cmd() should be converted to use it instead? thanks, greg k-h
Hi Greg, On 1/7/22 13:06, Greg KH wrote: > > We have a usb core function that handles these "short reads are an > error" issue. Perhaps usbnet_read_cmd() should be converted to use it > instead? > I thought about it. I am not sure, that there are no callers, that expect various length data. I remember, that I met such problem in atusb driver, but it uses plain usb API. I believe, we can provide new usbnet API, that will use usb_control_msg_{recv,send} and сarefully convert drivers to use it. When there won't be any callers of the old one we can just rename it. I might be missing something about usbnet, so, please, correct me if I am wrong here :) With regards, Pavel Skripkin
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Fri, 7 Jan 2022 01:57:16 +0300 you wrote: > Syzbot reported uninit value in mcs7830_bind(). The problem was in > missing validation check for bytes read via usbnet_read_cmd(). > > usbnet_read_cmd() internally calls usb_control_msg(), that returns > number of bytes read. Code should validate that requested number of bytes > was actually read. > > [...] Here is the summary with links: - net: mcs7830: handle usb read errors properly https://git.kernel.org/netdev/net/c/d668769eb9c5 You are awesome, thank you!
diff --git a/drivers/net/usb/mcs7830.c b/drivers/net/usb/mcs7830.c index 326cc4e749d8..fdda0616704e 100644 --- a/drivers/net/usb/mcs7830.c +++ b/drivers/net/usb/mcs7830.c @@ -108,8 +108,16 @@ static const char driver_name[] = "MOSCHIP usb-ethernet driver"; static int mcs7830_get_reg(struct usbnet *dev, u16 index, u16 size, void *data) { - return usbnet_read_cmd(dev, MCS7830_RD_BREQ, MCS7830_RD_BMREQ, - 0x0000, index, data, size); + int ret; + + ret = usbnet_read_cmd(dev, MCS7830_RD_BREQ, MCS7830_RD_BMREQ, + 0x0000, index, data, size); + if (ret < 0) + return ret; + else if (ret < size) + return -ENODATA; + + return ret; } static int mcs7830_set_reg(struct usbnet *dev, u16 index, u16 size, const void *data)
Syzbot reported uninit value in mcs7830_bind(). The problem was in missing validation check for bytes read via usbnet_read_cmd(). usbnet_read_cmd() internally calls usb_control_msg(), that returns number of bytes read. Code should validate that requested number of bytes was actually read. So, this patch adds missing size validation check inside mcs7830_get_reg() to prevent uninit value bugs CC: Arnd Bergmann <arnd@arndb.de> Reported-and-tested-by: syzbot+003c0a286b9af5412510@syzkaller.appspotmail.com Fixes: 2a36d7083438 ("USB: driver for mcs7830 (aka DeLOCK) USB ethernet adapter") Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- @Arnd, I am not sure about mcs7830_get_rev() function. Is get_reg(22, 2) == 1 valid read? If so, I think, we should call usbnet_read_cmd() directly here, since other callers care only about negative error values. Thanks --- drivers/net/usb/mcs7830.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)