| Message ID | 20210813160108.17534-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: asix: fix uninit value in asix_mdio_read | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | fail | 2 blamed authors not CCed: vpalatin@chromium.org robert.foss@collabora.com; 2 maintainers not CCed: vpalatin@chromium.org robert.foss@collabora.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 14 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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
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
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;
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(-)