| Message ID | 20210814135505.11920-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] net: asix: fix uninit value bugs | expand |
On Sat, Aug 14, 2021 at 04:55:05PM +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 handling and just continue the loop instead of > checking uninit value. > > Also, same loop was used in 3 other functions. Fixed uninit value bug > in them too. Hi Pavel Which suggests it might make sense to refactor the code to make a helper? I will leave you to decide if you want to do that. The code does looks correct now. Andrew
On 8/14/21 6:36 PM, Andrew Lunn wrote: > On Sat, Aug 14, 2021 at 04:55:05PM +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 handling and just continue the loop instead of >> checking uninit value. >> >> Also, same loop was used in 3 other functions. Fixed uninit value bug >> in them too. > > Hi Pavel > > Which suggests it might make sense to refactor the code to make a > helper? I will leave you to decide if you want to do that. It makes sense. Will add a helper function in v4. Thank you for suggestion and review! > > The code does looks correct now. > With regards, Pavel Skripkin
On 8/14/21 6:36 PM, Andrew Lunn wrote: > On Sat, Aug 14, 2021 at 04:55:05PM +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 handling and just continue the loop instead of >> checking uninit value. >> >> Also, same loop was used in 3 other functions. Fixed uninit value bug >> in them too. > > Hi Pavel > > Which suggests it might make sense to refactor the code to make a > helper? I will leave you to decide if you want to do that. > > The code does looks correct now. > > Andrew > I noticed strange thing. For example: driver looped 30 times, there wasn't any errors with usb transfer, but Host_En bit is not set. Datasheet says, that if Host_En is not set, that means software access will be ignored. Driver code doesn't handle this situation. We only check if ret is -ENODEV or -ETIMEOUT. I guess, next register access will fail, but anyway, does it make sense to return when Host_En bit is not set? With regards, Pavel Skripkin
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c index ac92bc52a85e..f0ee35edb746 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 ret; + int i; 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; @@ -506,21 +513,27 @@ static int __asix_mdio_write(struct net_device *netdev, int phy_id, int loc, struct usbnet *dev = netdev_priv(netdev); __le16 res = cpu_to_le16(val); u8 smsr; - int i = 0; int ret; + int i; netdev_dbg(dev->net, "asix_mdio_write() phy_id=0x%02x, loc=0x%02x, val=0x%04x\n", phy_id, loc, val); mutex_lock(&dev->phy_mutex); - do { + for (i = 0; i < 30; ++i) { ret = asix_set_sw_mii(dev, 0); if (ret == -ENODEV) 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) goto out; @@ -562,18 +575,25 @@ int asix_mdio_read_nopm(struct net_device *netdev, int phy_id, int loc) struct usbnet *dev = netdev_priv(netdev); __le16 res; u8 smsr; - int i = 0; int ret; + int i; mutex_lock(&dev->phy_mutex); - do { + for (i = 0; i < 30; ++i) { ret = asix_set_sw_mii(dev, 1); if (ret == -ENODEV || ret == -ETIMEDOUT) break; usleep_range(1000, 1100); ret = asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG, 0, 0, 1, &smsr, 1); - } 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; @@ -596,21 +616,28 @@ asix_mdio_write_nopm(struct net_device *netdev, int phy_id, int loc, int val) struct usbnet *dev = netdev_priv(netdev); __le16 res = cpu_to_le16(val); u8 smsr; - int i = 0; int ret; + int i; netdev_dbg(dev->net, "asix_mdio_write() phy_id=0x%02x, loc=0x%02x, val=0x%04x\n", phy_id, loc, val); mutex_lock(&dev->phy_mutex); - do { + for (i = 0; i < 30; ++i) { ret = asix_set_sw_mii(dev, 1); if (ret == -ENODEV) break; usleep_range(1000, 1100); ret = asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG, 0, 0, 1, &smsr, 1); - } 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) { mutex_unlock(&dev->phy_mutex); return;
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 handling and just continue the loop instead of checking uninit value. Also, same loop was used in 3 other functions. Fixed uninit value bug in them too. Cc: Robert Foss <robert.foss@collabora.com> Fixes: d9fe64e51114 ("net: asix: Add in_pm parameter") Reported-by: syzbot+a631ec9e717fb0423053@syzkaller.appspotmail.com Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- Changes in v2: 1. Fixed previous wrong approach and changed while loop to for loop 2. Reported-and-tested-by: tag removed, since KMSAN tests can be false positive. Used Reported-by instead. Changes in v3: 1. Addressed uninit value bugs in asix_mdio_write(), asix_mdio_read_nopm() and asix_mdio_write_nopm() 2. Moved i after ret to reverse xmas tree style 3. Fixed Fixes: tag --- drivers/net/usb/asix_common.c | 51 ++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 12 deletions(-)