This patch series aims to fix various issues throughout the QinHeng CH9200 driver. This driver fails to handle various failures, which in one case has lead to a uninit access bug found via syzbot. Upon reviewing the driver I fixed a few more issues which I have included in this patch series.
Parts of this series are the product of discussions and suggestions I had from others like Andrew Lunn, Simon Horman and Jakub Kicinski you can view those discussions below:
Link: https://lore.kernel.org/all/20250319112156.48312-1-qasdev00@gmail.com Link: https://lore.kernel.org/all/20250218002443.11731-1-qasdev00@gmail.com/ Link: https://lore.kernel.org/all/20250311161157.49065-1-qasdev00@gmail.com/
Qasim Ijaz (5): fix uninitialised access bug during mii_nway_restart remove extraneous return that prevents error propagation fail fast on control_read() failures during get_mac_address() add missing error handling in ch9200_bind() avoid triggering NWay restart on non-zero PHY ID
drivers/net/usb/ch9200.c | 61 ++++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 21 deletions(-)
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. We return 0 on control_read() failure here because returning a negative may trigger the "bmcr & BMCR_ANENABLE" check within mii_nway_restart.
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/usb/ch9200.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c index f69d9b902da0..4f29ecf2240a 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 < 0) + return 0;
return (buff[0] | buff[1] << 8); }
The control_write() function sets err to -EINVAL however there is an incorrectly placed 'return 0' statement after it which stops the propogation of the error.
Fix this issue by removing the 'return 0'.
Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices") Reviewed-by: Simon Horman horms@kernel.org Signed-off-by: Qasim Ijaz qasdev00@gmail.com --- drivers/net/usb/ch9200.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c index 4f29ecf2240a..61eb6c207eb1 100644 --- a/drivers/net/usb/ch9200.c +++ b/drivers/net/usb/ch9200.c @@ -168,8 +168,6 @@ static int control_write(struct usbnet *dev, unsigned char request, err = -EINVAL; kfree(buf);
- return 0; - err_out: return err; }
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH 2/5] net: ch9200: remove extraneous return that prevents error propagation Link: https://lore.kernel.org/stable/20250412183829.41342-3-qasdev00%40gmail.com
The get_mac_address() function has an issue 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.
Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices") Reviewed-by: Simon Horman horms@kernel.org Signed-off-by: Qasim Ijaz qasdev00@gmail.com --- drivers/net/usb/ch9200.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c index 61eb6c207eb1..4f1d2e9045a9 100644 --- a/drivers/net/usb/ch9200.c +++ b/drivers/net/usb/ch9200.c @@ -304,24 +304,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 < 0) + 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 < 0) + 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 < 0) + return rd_mac_len;
data[0] = mac_addr[5]; data[1] = mac_addr[4]; @@ -330,7 +333,7 @@ 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) @@ -386,6 +389,9 @@ static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf) CONTROL_TIMEOUT_MS);
retval = get_mac_address(dev, addr); + if (retval < 0) + return retval; + eth_hw_addr_set(dev->net, addr);
return retval;
The ch9200_bind() function has no error handling for any control_write() calls.
Fix this by checking if any control_write() call fails and propagate the error to the caller.
Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices") Reviewed-by: Simon Horman horms@kernel.org Signed-off-by: Qasim Ijaz qasdev00@gmail.com --- drivers/net/usb/ch9200.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c index 4f1d2e9045a9..187bbfc991f5 100644 --- a/drivers/net/usb/ch9200.c +++ b/drivers/net/usb/ch9200.c @@ -338,12 +338,12 @@ static int get_mac_address(struct usbnet *dev, unsigned char *data)
static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf) { - int retval = 0; + int retval; unsigned char data[2]; u8 addr[ETH_ALEN];
retval = usbnet_get_endpoints(dev, intf); - if (retval) + if (retval < 0) return retval;
dev->mii.dev = dev->net; @@ -361,32 +361,44 @@ 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 < 0) + 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 < 0) + 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 < 0) + 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 < 0) + 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 < 0) + 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 < 0) + return retval;
retval = get_mac_address(dev, addr); if (retval < 0) @@ -394,7 +406,7 @@ static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf)
eth_hw_addr_set(dev->net, addr);
- return retval; + return 0; }
static const struct driver_info ch9200_info = {
During ch9200_mdio_read if the phy_id is not 0 -ENODEV is returned.
In certain cases such as in mii_nway_restart returning a negative such as -ENODEV triggers the "bmcr & BMCR_ANENABLE" check, we should avoid this on error and just end the function.
To address this just return 0.
Signed-off-by: Qasim Ijaz qasdev00@gmail.com --- drivers/net/usb/ch9200.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c index 187bbfc991f5..281800bb2ff2 100644 --- a/drivers/net/usb/ch9200.c +++ b/drivers/net/usb/ch9200.c @@ -182,7 +182,7 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc) __func__, phy_id, loc);
if (phy_id != 0) - return -ENODEV; + return 0;
ret = control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02, CONTROL_TIMEOUT_MS);
linux-stable-mirror@lists.linaro.org
-
kernel test robot -
Qasim Ijaz