Re: [PATCH v2] net: asix: fix uninit value in asix_mdio_read
From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-08-13 22:52:32
Also in:
linux-usb, lkml
On Sat, 14 Aug 2021 01:42:19 +0300 Pavel Skripkin wrote:
Syzbot reported uninit-value in asix_mdio_read(). The problem was in
missing error handling. asix_read_cmd() should initialize passed stack
variable smsr, but it can fail in some cases. Then while condidition
checks possibly uninit smsr variable.
Since smsr is uninitialized stack variable, driver can misbehave,
because smsr will be random in case of asix_read_cmd() failure.
Fix it by adding error cheking and just continue the loop instead of
checking uninit value.
Fixes: 8a46f665833a ("net: asix: Avoid looping when the device is disconnected")This is not the right tag, the Fixes tag should point to the commit where the problem is introduced. Robert/Vincent only added some error checking, the issue was there before, right? Once you locate the right starting point for the fix please make sure to add the author to CC. Thanks!
Reported-by: syzbot+a631ec9e717fb0423053@syzkaller.appspotmail.com Signed-off-by: Pavel Skripkin <redacted>
quoted hunk ↗ jump to hunk
drivers/net/usb/asix_common.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c index ac92bc52a85e..7019c25e591c 100644 --- a/drivers/net/usb/asix_common.c +++ b/drivers/net/usb/asix_common.c@@ -468,18 +468,25 @@ int asix_mdio_read(struct net_device *netdev, int phy_id, int loc) struct usbnet *dev = netdev_priv(netdev); __le16 res; u8 smsr; - int i = 0; + int i; int ret;
nit: move i after ret, reverse xmas tree style
mutex_lock(&dev->phy_mutex);
- do {
+ for (i = 0; i < 30; ++i) {
ret = asix_set_sw_mii(dev, 0);
if (ret == -ENODEV || ret == -ETIMEDOUT)
break;
usleep_range(1000, 1100);
ret = asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG,
0, 0, 1, &smsr, 0);
- } while (!(smsr & AX_HOST_EN) && (i++ < 30) && (ret != -ENODEV));
+ if (ret == -ENODEV)
+ break;
+ else if (ret < 0)
+ continue;
+ else if (smsr & AX_HOST_EN)
+ break;
+ }
+
if (ret == -ENODEV || ret == -ETIMEDOUT) {
mutex_unlock(&dev->phy_mutex);
return ret;Code LGTM, do other functions which Robert/Vincent touched not need the same treatment tho?