net: mcs7830: handle usb read errors properly
Subsystems (4)
PATCH 1/1 net: mcs7830: handle usb read errors properly
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
---
@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(-)
drivers/net/usb/mcs7830.c +10 -2
@@ -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)
Signature · 1 line
2.34.1
Quoted 8 lines · Pavel Skripkin
Syzbot reported uninit value in
mcs7830_bind(). The problem was in missing validation check for bytes read viausbnet_read_cmd().
usbnet_read_cmd()internally callsusb_control_msg(), that returns number of bytes read. Code should validate that requested number of bytes was actually read.
Quoted 6 lines
So, this patch adds missing size validation check inside
mcs7830_get_reg()to prevent uninit value bugsCC Arnd Bergmann <[email protected]>Reported-and-tested-by [email protected]Fixes 2a36d7083438 ("USB: driver for mcs7830 (aka DeLOCK) USB ethernet adapter")
Looks good to me.
Quoted 8 lines
Signed-off-by Pavel Skripkin <[email protected]>---
@Arnd, I am not sure about
mcs7830_get_rev()function.Is
get_reg(22, 2)== 1 valid read? If so, I think, we should callusbnet_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
Greg KH
maintainerQuoted 37 lines · Pavel Skripkin
Syzbot reported uninit value in
mcs7830_bind(). The problem was in missing validation check for bytes read viausbnet_read_cmd().
usbnet_read_cmd()internally callsusb_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 bugsCC Arnd Bergmann <[email protected]>Reported-and-tested-by [email protected]Fixes 2a36d7083438 ("USB: driver for mcs7830 (aka DeLOCK) USB ethernet adapter")Signed-off-by Pavel Skripkin <[email protected]>---
@Arnd, I am not sure about
mcs7830_get_rev()function.Is
get_reg(22, 2)== 1 valid read? If so, I think, we should callusbnet_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(-)drivers/net/usb/mcs7830.c +0
-0@@ -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) {
drivers/net/usb/mcs7830.c static const char driver_name[] = "MOSCHIP usb-ethernet driver"; · line 118
Quoted 10 lines
drivers/net/usb/mcs7830.c +8
-2-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,
Quoted 5 lines · Greg KH
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
This patch was applied to netdev/net.git (master) by Jakub Kicinski <[email protected]>:
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski <[email protected]>:
Quoted 8 lines · 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 viausbnet_read_cmd().
usbnet_read_cmd()internally callsusb_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!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html