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

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

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