* [PATCH] net: asix: fix uninit value in asix_mdio_read @ 2021-08-13 16:01 Pavel Skripkin 2021-08-13 22:23 ` Andrew Lunn 0 siblings, 1 reply; 12+ messages in thread From: Pavel Skripkin @ 2021-08-13 16:01 UTC (permalink / raw) To: davem, kuba, linux, himadrispandya, andrew Cc: linux-usb, netdev, linux-kernel, Pavel Skripkin, syzbot+a631ec9e717fb0423053 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") Reported-and-tested-by: syzbot+a631ec9e717fb0423053@syzkaller.appspotmail.com Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- drivers/net/usb/asix_common.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c index ac92bc52a85e..572ca3077f8f 100644 --- a/drivers/net/usb/asix_common.c +++ b/drivers/net/usb/asix_common.c @@ -479,7 +479,13 @@ int asix_mdio_read(struct net_device *netdev, int phy_id, int loc) 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) { + ++i; + continue; + } + } while (!(smsr & AX_HOST_EN) && (i++ < 30)); if (ret == -ENODEV || ret == -ETIMEDOUT) { mutex_unlock(&dev->phy_mutex); return ret; -- 2.32.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] net: asix: fix uninit value in asix_mdio_read 2021-08-13 16:01 [PATCH] net: asix: fix uninit value in asix_mdio_read Pavel Skripkin @ 2021-08-13 22:23 ` Andrew Lunn 2021-08-13 22:29 ` Pavel Skripkin 2021-08-13 22:42 ` [PATCH v2] " Pavel Skripkin 0 siblings, 2 replies; 12+ messages in thread From: Andrew Lunn @ 2021-08-13 22:23 UTC (permalink / raw) To: Pavel Skripkin Cc: davem, kuba, linux, himadrispandya, linux-usb, netdev, linux-kernel, syzbot+a631ec9e717fb0423053 On Fri, Aug 13, 2021 at 07:01:08PM +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") > Reported-and-tested-by: syzbot+a631ec9e717fb0423053@syzkaller.appspotmail.com > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > --- > drivers/net/usb/asix_common.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c > index ac92bc52a85e..572ca3077f8f 100644 > --- a/drivers/net/usb/asix_common.c > +++ b/drivers/net/usb/asix_common.c > @@ -479,7 +479,13 @@ int asix_mdio_read(struct net_device *netdev, int phy_id, int loc) > 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) { > + ++i; > + continue; > + } > + } while (!(smsr & AX_HOST_EN) && (i++ < 30)); No ret < 0, don't you end up with a double increment of i? So it will only retry 15 times, not 30? Humm. If ret < 0 is true, smsr is uninitialized? The continue statement causes a jump into the condition expression, where we evaluate smsr & AX_HOST_EN. Isn't this just as broken as the original version? Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: asix: fix uninit value in asix_mdio_read 2021-08-13 22:23 ` Andrew Lunn @ 2021-08-13 22:29 ` Pavel Skripkin 2021-08-13 22:42 ` [PATCH v2] " Pavel Skripkin 1 sibling, 0 replies; 12+ messages in thread From: Pavel Skripkin @ 2021-08-13 22:29 UTC (permalink / raw) To: Andrew Lunn Cc: davem, kuba, linux, himadrispandya, linux-usb, netdev, linux-kernel, syzbot+a631ec9e717fb0423053 On 8/14/21 1:23 AM, Andrew Lunn wrote: > On Fri, Aug 13, 2021 at 07:01:08PM +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") >> Reported-and-tested-by: syzbot+a631ec9e717fb0423053@syzkaller.appspotmail.com >> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> >> --- >> drivers/net/usb/asix_common.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c >> index ac92bc52a85e..572ca3077f8f 100644 >> --- a/drivers/net/usb/asix_common.c >> +++ b/drivers/net/usb/asix_common.c >> @@ -479,7 +479,13 @@ int asix_mdio_read(struct net_device *netdev, int phy_id, int loc) >> 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) { >> + ++i; >> + continue; >> + } >> + } while (!(smsr & AX_HOST_EN) && (i++ < 30)); > > No ret < 0, don't you end up with a double increment of i? So it will > only retry 15 times, not 30? > > Humm. > > If ret < 0 is true, smsr is uninitialized? The continue statement > causes a jump into the condition expression, where we evaluate smsr & > AX_HOST_EN. Isn't this just as broken as the original version? > > Andrew > Yes, you are right, I missed that, sorry. I will rewrote this loop into for loop in v2. Im wondering why this wrong patch passed KMSAN testing... With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] net: asix: fix uninit value in asix_mdio_read 2021-08-13 22:23 ` Andrew Lunn 2021-08-13 22:29 ` Pavel Skripkin @ 2021-08-13 22:42 ` Pavel Skripkin 2021-08-13 22:52 ` Jakub Kicinski 2021-08-13 22:57 ` [PATCH v2] net: asix: fix uninit value in asix_mdio_read Andrew Lunn 1 sibling, 2 replies; 12+ messages in thread From: Pavel Skripkin @ 2021-08-13 22:42 UTC (permalink / raw) To: davem, kuba, linux, himadrispandya, andrew Cc: linux-usb, netdev, linux-kernel, Pavel Skripkin, syzbot+a631ec9e717fb0423053 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") 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. --- 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; 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; -- 2.32.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] net: asix: fix uninit value in asix_mdio_read 2021-08-13 22:42 ` [PATCH v2] " Pavel Skripkin @ 2021-08-13 22:52 ` Jakub Kicinski 2021-08-14 13:55 ` [PATCH v3] net: asix: fix uninit value bugs Pavel Skripkin 2021-08-13 22:57 ` [PATCH v2] net: asix: fix uninit value in asix_mdio_read Andrew Lunn 1 sibling, 1 reply; 12+ messages in thread From: Jakub Kicinski @ 2021-08-13 22:52 UTC (permalink / raw) To: Pavel Skripkin Cc: davem, linux, himadrispandya, andrew, linux-usb, netdev, linux-kernel, syzbot+a631ec9e717fb0423053, Robert Foss, Vincent Palatin 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 <paskripkin@gmail.com> > 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] net: asix: fix uninit value bugs 2021-08-13 22:52 ` Jakub Kicinski @ 2021-08-14 13:55 ` Pavel Skripkin 2021-08-14 15:36 ` Andrew Lunn 0 siblings, 1 reply; 12+ messages in thread From: Pavel Skripkin @ 2021-08-14 13:55 UTC (permalink / raw) To: davem, kuba, linux, andrew Cc: robert.foss, linux-usb, netdev, linux-kernel, Pavel Skripkin, syzbot+a631ec9e717fb0423053 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(-) 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; -- 2.32.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3] net: asix: fix uninit value bugs 2021-08-14 13:55 ` [PATCH v3] net: asix: fix uninit value bugs Pavel Skripkin @ 2021-08-14 15:36 ` Andrew Lunn 2021-08-14 15:40 ` Pavel Skripkin ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Andrew Lunn @ 2021-08-14 15:36 UTC (permalink / raw) To: Pavel Skripkin Cc: davem, kuba, linux, robert.foss, linux-usb, netdev, linux-kernel, syzbot+a631ec9e717fb0423053 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] net: asix: fix uninit value bugs 2021-08-14 15:36 ` Andrew Lunn @ 2021-08-14 15:40 ` Pavel Skripkin 2021-08-14 16:20 ` Pavel Skripkin 2021-08-17 16:37 ` [PATCH v4] " Pavel Skripkin 2 siblings, 0 replies; 12+ messages in thread From: Pavel Skripkin @ 2021-08-14 15:40 UTC (permalink / raw) To: Andrew Lunn Cc: davem, kuba, linux, robert.foss, linux-usb, netdev, linux-kernel, syzbot+a631ec9e717fb0423053 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] net: asix: fix uninit value bugs 2021-08-14 15:36 ` Andrew Lunn 2021-08-14 15:40 ` Pavel Skripkin @ 2021-08-14 16:20 ` Pavel Skripkin 2021-08-17 16:37 ` [PATCH v4] " Pavel Skripkin 2 siblings, 0 replies; 12+ messages in thread From: Pavel Skripkin @ 2021-08-14 16:20 UTC (permalink / raw) To: Andrew Lunn Cc: davem, kuba, linux, robert.foss, linux-usb, netdev, linux-kernel, syzbot+a631ec9e717fb0423053 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4] net: asix: fix uninit value bugs 2021-08-14 15:36 ` Andrew Lunn 2021-08-14 15:40 ` Pavel Skripkin 2021-08-14 16:20 ` Pavel Skripkin @ 2021-08-17 16:37 ` Pavel Skripkin 2021-08-18 10:50 ` patchwork-bot+netdevbpf 2 siblings, 1 reply; 12+ messages in thread From: Pavel Skripkin @ 2021-08-17 16:37 UTC (permalink / raw) To: davem, kuba, linux, andrew, himadrispandya Cc: robert.foss, freddy, linux-usb, netdev, linux-kernel, Pavel Skripkin, syzbot+a631ec9e717fb0423053 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. Added helper function for checking Host_En bit, since wrong loop was used in 4 functions and there is no need in copy-pasting code parts. 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 Changes in v4: 1. Added helper for checking Host_En bit, since wrong loop was used in 4 functions. (Suggested by Andrew Lunn) --- drivers/net/usb/asix_common.c | 70 +++++++++++++++-------------------- 1 file changed, 30 insertions(+), 40 deletions(-) diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c index ac92bc52a85e..38cda590895c 100644 --- a/drivers/net/usb/asix_common.c +++ b/drivers/net/usb/asix_common.c @@ -63,6 +63,29 @@ void asix_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value, u16 index, value, index, data, size); } +static int asix_check_host_enable(struct usbnet *dev, int in_pm) +{ + int i, ret; + u8 smsr; + + for (i = 0; i < 30; ++i) { + ret = asix_set_sw_mii(dev, in_pm); + if (ret == -ENODEV || ret == -ETIMEDOUT) + break; + usleep_range(1000, 1100); + ret = asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG, + 0, 0, 1, &smsr, in_pm); + if (ret == -ENODEV) + break; + else if (ret < 0) + continue; + else if (smsr & AX_HOST_EN) + break; + } + + return ret; +} + static void reset_asix_rx_fixup_info(struct asix_rx_fixup_info *rx) { /* Reset the variables that have a lifetime outside of @@ -467,19 +490,11 @@ 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; mutex_lock(&dev->phy_mutex); - do { - 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)); + + ret = asix_check_host_enable(dev, 0); if (ret == -ENODEV || ret == -ETIMEDOUT) { mutex_unlock(&dev->phy_mutex); return ret; @@ -505,23 +520,14 @@ 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; 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 { - 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)); + ret = asix_check_host_enable(dev, 0); if (ret == -ENODEV) goto out; @@ -561,19 +567,11 @@ 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; mutex_lock(&dev->phy_mutex); - do { - 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)); + + ret = asix_check_host_enable(dev, 1); if (ret == -ENODEV || ret == -ETIMEDOUT) { mutex_unlock(&dev->phy_mutex); return ret; @@ -595,22 +593,14 @@ 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; 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 { - 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)); + + ret = asix_check_host_enable(dev, 1); if (ret == -ENODEV) { mutex_unlock(&dev->phy_mutex); return; -- 2.32.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4] net: asix: fix uninit value bugs 2021-08-17 16:37 ` [PATCH v4] " Pavel Skripkin @ 2021-08-18 10:50 ` patchwork-bot+netdevbpf 0 siblings, 0 replies; 12+ messages in thread From: patchwork-bot+netdevbpf @ 2021-08-18 10:50 UTC (permalink / raw) To: Pavel Skripkin Cc: davem, kuba, linux, andrew, himadrispandya, robert.foss, freddy, linux-usb, netdev, linux-kernel, syzbot+a631ec9e717fb0423053 Hello: This patch was applied to netdev/net.git (refs/heads/master): On Tue, 17 Aug 2021 19:37:23 +0300 you 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. > > [...] Here is the summary with links: - [v4] net: asix: fix uninit value bugs https://git.kernel.org/netdev/net/c/a786e3195d6a You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] net: asix: fix uninit value in asix_mdio_read 2021-08-13 22:42 ` [PATCH v2] " Pavel Skripkin 2021-08-13 22:52 ` Jakub Kicinski @ 2021-08-13 22:57 ` Andrew Lunn 1 sibling, 0 replies; 12+ messages in thread From: Andrew Lunn @ 2021-08-13 22:57 UTC (permalink / raw) To: Pavel Skripkin Cc: davem, kuba, linux, himadrispandya, linux-usb, netdev, linux-kernel, syzbot+a631ec9e717fb0423053 > 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; > > 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; > + } > + Yes, this looks good. And Jakub is correct, there are 3 other bits of similar code you should look at. Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-08-18 10:50 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-13 16:01 [PATCH] net: asix: fix uninit value in asix_mdio_read Pavel Skripkin 2021-08-13 22:23 ` Andrew Lunn 2021-08-13 22:29 ` Pavel Skripkin 2021-08-13 22:42 ` [PATCH v2] " Pavel Skripkin 2021-08-13 22:52 ` Jakub Kicinski 2021-08-14 13:55 ` [PATCH v3] net: asix: fix uninit value bugs Pavel Skripkin 2021-08-14 15:36 ` Andrew Lunn 2021-08-14 15:40 ` Pavel Skripkin 2021-08-14 16:20 ` Pavel Skripkin 2021-08-17 16:37 ` [PATCH v4] " Pavel Skripkin 2021-08-18 10:50 ` patchwork-bot+netdevbpf 2021-08-13 22:57 ` [PATCH v2] net: asix: fix uninit value in asix_mdio_read Andrew Lunn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).