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 <[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 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;
+
+ return ret;
}
static int mcs7830_set_reg(struct usbnet *dev, u16 index, u16 size, const void *data)
--
2.34.1
On Thu, Jan 6, 2022 at 5:57 PM Pavel Skripkin <[email protected]> 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 <[email protected]>
> Reported-and-tested-by: [email protected]
> Fixes: 2a36d7083438 ("USB: driver for mcs7830 (aka DeLOCK) USB ethernet adapter")
Looks good to me.
Reviewed-by: Arnd Bergmann <[email protected]>
> 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 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 <[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 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 <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html