diff mbox series

[SRU,F,1/2] net: asix: fix uninit value bugs

Message ID 20241025132720.2838693-2-koichiro.den@canonical.com
State New
Headers show
Series CVE-2021-47101 | expand

Commit Message

Koichiro Den Oct. 25, 2024, 1:27 p.m. UTC 
From: Pavel Skripkin <paskripkin@gmail.com>

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>
Signed-off-by: David S. Miller <davem@davemloft.net>
(backported from commit a786e3195d6af183033e86f0518ffd2c51c0e8ac)
[koichiroden: Adjusted context due to missing commit d275afb66371 ("net:
usb: asix: add error handling for asix_mdio_* functions"), which in turn
depends on e532a096be0e ("net: usb: asix: ax88772: add phylib support").
Ref. [PATCH net-next v2 0/8] port asix ax88772 to the PHYlib
https://lore.kernel.org/linux-usb/20210607082727.26045-1-o.rempel@pengutronix.de/]
CVE-2021-47101
Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/net/usb/asix_common.c | 71 +++++++++++++++--------------------
 1 file changed, 31 insertions(+), 40 deletions(-)

Comments

Manuel Diewald Oct. 25, 2024, 1:49 p.m. UTC | #1 
On Fri, Oct 25, 2024 at 10:27:14PM +0900, Koichiro Den wrote:
> From: Pavel Skripkin <paskripkin@gmail.com>
> 
> 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>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> (backported from commit a786e3195d6af183033e86f0518ffd2c51c0e8ac)
> [koichiroden: Adjusted context due to missing commit d275afb66371 ("net:
> usb: asix: add error handling for asix_mdio_* functions"), which in turn
> depends on e532a096be0e ("net: usb: asix: ax88772: add phylib support").
> Ref. [PATCH net-next v2 0/8] port asix ax88772 to the PHYlib
> https://lore.kernel.org/linux-usb/20210607082727.26045-1-o.rempel@pengutronix.de/]
> CVE-2021-47101
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> ---
>  drivers/net/usb/asix_common.c | 71 +++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
> index 7bc6e8f856fe..12ce52600eaf 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;

There is a follow-up fix available upstream that fixes the return value
of this new function:

d1652b70d07c asix: fix wrong return value in asix_check_host_enable()

I think we should include it in this patchset.
Koichiro Den Oct. 28, 2024, 6:41 a.m. UTC | #2 
On Fri, Oct 25, 2024 at 03:49:53PM +0200, Manuel Diewald wrote:
> On Fri, Oct 25, 2024 at 10:27:14PM +0900, Koichiro Den wrote:
> > From: Pavel Skripkin <paskripkin@gmail.com>
> > 
> > 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>
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> > (backported from commit a786e3195d6af183033e86f0518ffd2c51c0e8ac)
> > [koichiroden: Adjusted context due to missing commit d275afb66371 ("net:
> > usb: asix: add error handling for asix_mdio_* functions"), which in turn
> > depends on e532a096be0e ("net: usb: asix: ax88772: add phylib support").
> > Ref. [PATCH net-next v2 0/8] port asix ax88772 to the PHYlib
> > https://lore.kernel.org/linux-usb/20210607082727.26045-1-o.rempel@pengutronix.de/]
> > CVE-2021-47101
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> > ---
> >  drivers/net/usb/asix_common.c | 71 +++++++++++++++--------------------
> >  1 file changed, 31 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
> > index 7bc6e8f856fe..12ce52600eaf 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;
> 
> There is a follow-up fix available upstream that fixes the return value
> of this new function:
> 
> d1652b70d07c asix: fix wrong return value in asix_check_host_enable()
> 
> I think we should include it in this patchset.

Thank you for catching this. I'll send v2 soon.

> 
> -- 
>  Manuel
diff mbox series

Patch

diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 7bc6e8f856fe..12ce52600eaf 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
@@ -445,19 +468,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;
@@ -478,22 +493,14 @@  void asix_mdio_write(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, 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) {
 		mutex_unlock(&dev->phy_mutex);
 		return;
@@ -509,19 +516,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;
@@ -543,22 +542,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;