In mii_nway_restart() during the line:

bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);

The code attempts to call mii->mdio_read which is ch9200_mdio_read().

ch9200_mdio_read() utilises a local buffer, which is initialised with control_read():

unsigned char buff[2];

However buff is conditionally initialised inside control_read():

if (err == size) { memcpy(data, buf, size); }

If the condition of "err == size" is not met, then buff remains uninitialised. Once this happens the uninitialised buff is accessed and returned during ch9200_mdio_read():

return (buff[0] | buff[1] << 8);

The problem stems from the fact that ch9200_mdio_read() ignores the return value of control_read(), leading to uinit-access of buff.

To fix this we should check the return value of control_read() and return early on error.

Furthermore the get_mac_address() function has a similar problem where it does not directly check the return value of each control_read(), instead it sums up the return values and checks them all at the end which means if any call to control_read() fails the function just continues on.

Handle this by validating the return value of each call and fail fast and early instead of continuing.

Lastly ch9200_bind() ignores the return values of multiple control_write() calls.

Validate each control_write() call to ensure it succeeds before continuing with the next call.

Reported-by: syzbot syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=3361c2d6f78a3e0892f9 Tested-by: syzbot syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices") Cc: stable@vger.kernel.org Signed-off-by: Qasim Ijaz qasdev00@gmail.com --- drivers/net/mii.c | 2 ++ drivers/net/usb/ch9200.c | 55 +++++++++++++++++++++++++++------------- 2 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/drivers/net/mii.c b/drivers/net/mii.c index 37bc3131d31a..e305bf0f1d04 100644 --- a/drivers/net/mii.c +++ b/drivers/net/mii.c @@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)

/* if autoneg is off, it's an error */ bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR); + if (bmcr < 0) + return bmcr;

if (bmcr & BMCR_ANENABLE) { bmcr |= BMCR_ANRESTART; diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c index f69d9b902da0..e938501a1fc8 100644 --- a/drivers/net/usb/ch9200.c +++ b/drivers/net/usb/ch9200.c @@ -178,6 +178,7 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc) { struct usbnet *dev = netdev_priv(netdev); unsigned char buff[2]; + int ret;

netdev_dbg(netdev, "%s phy_id:%02x loc:%02x\n", __func__, phy_id, loc); @@ -185,8 +186,10 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc) if (phy_id != 0) return -ENODEV;

- control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02, - CONTROL_TIMEOUT_MS); + ret = control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02, + CONTROL_TIMEOUT_MS); + if (ret != 2) + return ret;

return (buff[0] | buff[1] << 8); } @@ -303,24 +306,27 @@ static int ch9200_rx_fixup(struct usbnet *dev, struct sk_buff *skb)

static int get_mac_address(struct usbnet *dev, unsigned char *data) { - int err = 0; unsigned char mac_addr[0x06]; - int rd_mac_len = 0; + int rd_mac_len;

netdev_dbg(dev->net, "%s:\n\tusbnet VID:%0x PID:%0x\n", __func__, le16_to_cpu(dev->udev->descriptor.idVendor), le16_to_cpu(dev->udev->descriptor.idProduct));

- memset(mac_addr, 0, sizeof(mac_addr)); - rd_mac_len = control_read(dev, REQUEST_READ, 0, - MAC_REG_STATION_L, mac_addr, 0x02, - CONTROL_TIMEOUT_MS); - rd_mac_len += control_read(dev, REQUEST_READ, 0, MAC_REG_STATION_M, - mac_addr + 2, 0x02, CONTROL_TIMEOUT_MS); - rd_mac_len += control_read(dev, REQUEST_READ, 0, MAC_REG_STATION_H, - mac_addr + 4, 0x02, CONTROL_TIMEOUT_MS); - if (rd_mac_len != ETH_ALEN) - err = -EINVAL; + rd_mac_len = control_read(dev, REQUEST_READ, 0, MAC_REG_STATION_L, + mac_addr, 0x02, CONTROL_TIMEOUT_MS); + if (rd_mac_len != 2) + return rd_mac_len; + + rd_mac_len = control_read(dev, REQUEST_READ, 0, MAC_REG_STATION_M, + mac_addr + 2, 0x02, CONTROL_TIMEOUT_MS); + if (rd_mac_len != 2) + return rd_mac_len; + + rd_mac_len = control_read(dev, REQUEST_READ, 0, MAC_REG_STATION_H, + mac_addr + 4, 0x02, CONTROL_TIMEOUT_MS); + if (rd_mac_len != 2) + return rd_mac_len;

data[0] = mac_addr[5]; data[1] = mac_addr[4]; @@ -329,12 +335,12 @@ static int get_mac_address(struct usbnet *dev, unsigned char *data) data[4] = mac_addr[1]; data[5] = mac_addr[0];

- return err; + return 0; }

static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf) { - int retval = 0; + int retval; unsigned char data[2]; u8 addr[ETH_ALEN];

@@ -357,37 +363,52 @@ static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf) data[1] = 0x0F; retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_THRESHOLD, data, 0x02, CONTROL_TIMEOUT_MS); + if (retval) + return retval;

data[0] = 0xA0; data[1] = 0x90; retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_FIFO_DEPTH, data, 0x02, CONTROL_TIMEOUT_MS); + if (retval) + return retval;

data[0] = 0x30; data[1] = 0x00; retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_PAUSE, data, 0x02, CONTROL_TIMEOUT_MS); + if (retval) + return retval;

data[0] = 0x17; data[1] = 0xD8; retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_FLOW_CONTROL, data, 0x02, CONTROL_TIMEOUT_MS); + if (retval) + return retval;

/* Undocumented register */ data[0] = 0x01; data[1] = 0x00; retval = control_write(dev, REQUEST_WRITE, 0, 254, data, 0x02, CONTROL_TIMEOUT_MS); + if (retval) + return retval;

data[0] = 0x5F; data[1] = 0x0D; retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_CTRL, data, 0x02, CONTROL_TIMEOUT_MS); + if (retval) + return retval;

retval = get_mac_address(dev, addr); + if (retval) + return retval; + eth_hw_addr_set(dev->net, addr);

- return retval; + return 0; }

static const struct driver_info ch9200_info = {