linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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

* 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

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

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