[PATCH] net: asix: fix uninit value in asix_mdio_read

[PATCH] net: asix: fix uninit value in asix_mdio_read

[Pavel Skripkin]

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

Re: [PATCH] net: asix: fix uninit value in asix_mdio_read

[Andrew Lunn]

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

Re: [PATCH] net: asix: fix uninit value in asix_mdio_read

[Pavel Skripkin]

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

[PATCH v2] net: asix: fix uninit value in asix_mdio_read

[Pavel Skripkin]

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

Re: [PATCH v2] net: asix: fix uninit value in asix_mdio_read

[Jakub Kicinski]

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?

[PATCH v3] net: asix: fix uninit value bugs

[Pavel Skripkin]

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

Re: [PATCH v3] net: asix: fix uninit value bugs

[Andrew Lunn]

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

Re: [PATCH v3] net: asix: fix uninit value bugs

[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.

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

Re: [PATCH v3] net: asix: fix uninit value bugs

[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

[PATCH v4] net: asix: fix uninit value bugs

[Pavel Skripkin]

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

Re: [PATCH v4] net: asix: fix uninit value bugs

[patchwork-bot+netdevbpf]

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

Re: [PATCH v2] net: asix: fix uninit value in asix_mdio_read

[Andrew Lunn]

> 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