[PATCH] net: mcs7830: handle usb read errors properly

[PATCH] net: mcs7830: handle usb read errors properly

[Pavel Skripkin]

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;
+
+	return ret;
 }
 
 static int mcs7830_set_reg(struct usbnet *dev, u16 index, u16 size, const void *data)
-- 
2.34.1

Re: [PATCH] net: mcs7830: handle usb read errors properly

[Arnd Bergmann]

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

Re: [PATCH] net: mcs7830: handle usb read errors properly

[Greg KH]

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

Re: [PATCH] net: mcs7830: handle usb read errors properly

[Pavel Skripkin]

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

Re: [PATCH] net: mcs7830: handle usb read errors properly

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html