[PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Qasim Ijaz]

In mii_nway_restart() during the line:

        bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);

The code attempts to call mii->mdio_read which is ch9200_mdio_read().

ch9200_mdio_read() utilises a local buffer, which is initialised
with control_read():

        unsigned char buff[2];

However buff is conditionally initialised inside control_read():

        if (err == size) {
                memcpy(data, buf, size);
        }

If the condition of "err == size" is not met, then buff remains
uninitialised. Once this happens the uninitialised buff is accessed
and returned during ch9200_mdio_read():

        return (buff[0] | buff[1] << 8);

The problem stems from the fact that ch9200_mdio_read() ignores the
return value of control_read(), leading to uinit-access of buff.

To fix this we should check the return value of control_read()
and return early on error.

Reported-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=3361c2d6f78a3e0892f9
Tested-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
Cc: stable@vger.kernel.org
Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
---
 drivers/net/mii.c        | 2 ++
 drivers/net/usb/ch9200.c | 7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mii.c b/drivers/net/mii.c
index 37bc3131d31a..e305bf0f1d04 100644
--- a/drivers/net/mii.c
+++ b/drivers/net/mii.c
@@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)
 
 	/* if autoneg is off, it's an error */
 	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
+	if (bmcr < 0)
+		return bmcr;
 
 	if (bmcr & BMCR_ANENABLE) {
 		bmcr |= BMCR_ANRESTART;
diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
index f69d9b902da0..a206ffa76f1b 100644
--- a/drivers/net/usb/ch9200.c
+++ b/drivers/net/usb/ch9200.c
@@ -178,6 +178,7 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc)
 {
 	struct usbnet *dev = netdev_priv(netdev);
 	unsigned char buff[2];
+	int ret;
 
 	netdev_dbg(netdev, "%s phy_id:%02x loc:%02x\n",
 		   __func__, phy_id, loc);
@@ -185,8 +186,10 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc)
 	if (phy_id != 0)
 		return -ENODEV;
 
-	control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02,
-		     CONTROL_TIMEOUT_MS);
+	ret = control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02,
+			   CONTROL_TIMEOUT_MS);
+	if (ret < 0)
+		return ret;
 
 	return (buff[0] | buff[1] << 8);
 }
-- 
2.39.5

[PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Qasim Ijaz]

In mii_nway_restart() during the line:

        bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);

The code attempts to call mii->mdio_read which is ch9200_mdio_read().

ch9200_mdio_read() utilises a local buffer, which is initialised
with control_read():

        unsigned char buff[2];

However buff is conditionally initialised inside control_read():

        if (err == size) {
                memcpy(data, buf, size);
        }

If the condition of "err == size" is not met, then buff remains
uninitialised. Once this happens the uninitialised buff is accessed
and returned during ch9200_mdio_read():

        return (buff[0] | buff[1] << 8);

The problem stems from the fact that ch9200_mdio_read() ignores the
return value of control_read(), leading to uinit-access of buff.

To fix this we should check the return value of control_read()
and return early on error.

Reported-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=3361c2d6f78a3e0892f9
Tested-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
Cc: stable@vger.kernel.org
Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
---
 drivers/net/mii.c        | 2 ++
 drivers/net/usb/ch9200.c | 7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mii.c b/drivers/net/mii.c
index 37bc3131d31a..e305bf0f1d04 100644
--- a/drivers/net/mii.c
+++ b/drivers/net/mii.c
@@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)
 
 	/* if autoneg is off, it's an error */
 	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
+	if (bmcr < 0)
+		return bmcr;
 
 	if (bmcr & BMCR_ANENABLE) {
 		bmcr |= BMCR_ANRESTART;
diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
index f69d9b902da0..a206ffa76f1b 100644
--- a/drivers/net/usb/ch9200.c
+++ b/drivers/net/usb/ch9200.c
@@ -178,6 +178,7 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc)
 {
 	struct usbnet *dev = netdev_priv(netdev);
 	unsigned char buff[2];
+	int ret;
 
 	netdev_dbg(netdev, "%s phy_id:%02x loc:%02x\n",
 		   __func__, phy_id, loc);
@@ -185,8 +186,10 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc)
 	if (phy_id != 0)
 		return -ENODEV;
 
-	control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02,
-		     CONTROL_TIMEOUT_MS);
+	ret = control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02,
+			   CONTROL_TIMEOUT_MS);
+	if (ret < 0)
+		return ret;
 
 	return (buff[0] | buff[1] << 8);
 }
-- 
2.39.5

[PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Qasim Ijaz]

In mii_nway_restart() during the line:

        bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);

The code attempts to call mii->mdio_read which is ch9200_mdio_read().

ch9200_mdio_read() utilises a local buffer, which is initialised
with control_read():

        unsigned char buff[2];

However buff is conditionally initialised inside control_read():

        if (err == size) {
                memcpy(data, buf, size);
        }

If the condition of "err == size" is not met, then buff remains
uninitialised. Once this happens the uninitialised buff is accessed
and returned during ch9200_mdio_read():

        return (buff[0] | buff[1] << 8);

The problem stems from the fact that ch9200_mdio_read() ignores the
return value of control_read(), leading to uinit-access of buff.

To fix this we should check the return value of control_read()
and return early on error.

Reported-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=3361c2d6f78a3e0892f9
Tested-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
Cc: stable@vger.kernel.org
Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
---
 drivers/net/mii.c        | 2 ++
 drivers/net/usb/ch9200.c | 7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mii.c b/drivers/net/mii.c
index 37bc3131d31a..e305bf0f1d04 100644
--- a/drivers/net/mii.c
+++ b/drivers/net/mii.c
@@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)
 
 	/* if autoneg is off, it's an error */
 	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
+	if (bmcr < 0)
+		return bmcr;
 
 	if (bmcr & BMCR_ANENABLE) {
 		bmcr |= BMCR_ANRESTART;
diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
index f69d9b902da0..a206ffa76f1b 100644
--- a/drivers/net/usb/ch9200.c
+++ b/drivers/net/usb/ch9200.c
@@ -178,6 +178,7 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc)
 {
 	struct usbnet *dev = netdev_priv(netdev);
 	unsigned char buff[2];
+	int ret;
 
 	netdev_dbg(netdev, "%s phy_id:%02x loc:%02x\n",
 		   __func__, phy_id, loc);
@@ -185,8 +186,10 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc)
 	if (phy_id != 0)
 		return -ENODEV;
 
-	control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02,
-		     CONTROL_TIMEOUT_MS);
+	ret = control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02,
+			   CONTROL_TIMEOUT_MS);
+	if (ret < 0)
+		return ret;
 
 	return (buff[0] | buff[1] << 8);
 }
-- 
2.39.5

Re: [PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Simon Horman]

On Wed, Mar 19, 2025 at 11:21:53AM +0000, Qasim Ijaz wrote:
> In mii_nway_restart() during the line:
> 
>         bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
> 
> The code attempts to call mii->mdio_read which is ch9200_mdio_read().
> 
> ch9200_mdio_read() utilises a local buffer, which is initialised
> with control_read():
> 
>         unsigned char buff[2];
> 
> However buff is conditionally initialised inside control_read():
> 
>         if (err == size) {
>                 memcpy(data, buf, size);
>         }
> 
> If the condition of "err == size" is not met, then buff remains
> uninitialised. Once this happens the uninitialised buff is accessed
> and returned during ch9200_mdio_read():
> 
>         return (buff[0] | buff[1] << 8);
> 
> The problem stems from the fact that ch9200_mdio_read() ignores the
> return value of control_read(), leading to uinit-access of buff.
> 
> To fix this we should check the return value of control_read()
> and return early on error.
> 
> Reported-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=3361c2d6f78a3e0892f9
> Tested-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
> Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Jakub Kicinski]

On Wed, 19 Mar 2025 11:21:53 +0000 Qasim Ijaz wrote:
> --- a/drivers/net/mii.c
> +++ b/drivers/net/mii.c
> @@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)
>  
>  	/* if autoneg is off, it's an error */
>  	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
> +	if (bmcr < 0)
> +		return bmcr;
>  
>  	if (bmcr & BMCR_ANENABLE) {
>  		bmcr |= BMCR_ANRESTART;

We error check just one mdio_read() but there's a whole bunch of them
in this file. What's the expected behavior then? Are all of them buggy?

This patch should be split into core and driver parts.
-- 
pw-bot: cr

Re: [PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Qasim Ijaz]

On Tue, Mar 25, 2025 at 06:33:07AM -0700, Jakub Kicinski wrote:
> On Wed, 19 Mar 2025 11:21:53 +0000 Qasim Ijaz wrote:
> > --- a/drivers/net/mii.c
> > +++ b/drivers/net/mii.c
> > @@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)
> >  
> >  	/* if autoneg is off, it's an error */
> >  	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
> > +	if (bmcr < 0)
> > +		return bmcr;
> >  
> >  	if (bmcr & BMCR_ANENABLE) {
> >  		bmcr |= BMCR_ANRESTART;
> 
> We error check just one mdio_read() but there's a whole bunch of them
> in this file. What's the expected behavior then? Are all of them buggy?
>
 
Hi Jakub
    
Apologies for my delayed response, I had another look at this and I
think my patch may be off a bit. You are correct that there are multiple
mdio_read() calls and looking at the mii.c file we can see that calls to
functions like mdio_read (and a lot of others) dont check return values.
  
So in light of this I think a better patch would be to not edit the 
mii.c file at all and just make ch9200_mdio_read return 0 on     
error. This way if mdio_read fails and 0 is returned, the         
check for "bmcr & BMCR_ANENABLE" won't be triggered and mii_nway_restart
will just return 0 and end. If we return a negative on error it may
contain the exact bit the function checks.

Similiar to this patch:
<https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=c68b2c9eba38>

If this sounds good, should i send another patch series with all the
changes? 

> This patch should be split into core and driver parts.
> -- 
> pw-bot: cr

Re: [PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Jakub Kicinski]

On Thu, 10 Apr 2025 23:15:23 +0100 Qasim Ijaz wrote:
> Apologies for my delayed response, I had another look at this and I
> think my patch may be off a bit. You are correct that there are multiple
> mdio_read() calls and looking at the mii.c file we can see that calls to
> functions like mdio_read (and a lot of others) dont check return values.
>   
> So in light of this I think a better patch would be to not edit the 
> mii.c file at all and just make ch9200_mdio_read return 0 on     
> error. This way if mdio_read fails and 0 is returned, the         
> check for "bmcr & BMCR_ANENABLE" won't be triggered and mii_nway_restart
> will just return 0 and end. If we return a negative on error it may
> contain the exact bit the function checks.
> 
> Similiar to this patch:
> <https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=c68b2c9eba38>
> 
> If this sounds good, should i send another patch series with all the
> changes? 

SG

Re: [PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Andrew Lunn]

On Thu, Apr 10, 2025 at 11:15:23PM +0100, Qasim Ijaz wrote:
> On Tue, Mar 25, 2025 at 06:33:07AM -0700, Jakub Kicinski wrote:
> > On Wed, 19 Mar 2025 11:21:53 +0000 Qasim Ijaz wrote:
> > > --- a/drivers/net/mii.c
> > > +++ b/drivers/net/mii.c
> > > @@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)
> > >  
> > >  	/* if autoneg is off, it's an error */
> > >  	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
> > > +	if (bmcr < 0)
> > > +		return bmcr;
> > >  
> > >  	if (bmcr & BMCR_ANENABLE) {
> > >  		bmcr |= BMCR_ANRESTART;
> > 
> > We error check just one mdio_read() but there's a whole bunch of them
> > in this file. What's the expected behavior then? Are all of them buggy?
> >
>  
> Hi Jakub
>     
> Apologies for my delayed response, I had another look at this and I
> think my patch may be off a bit. You are correct that there are multiple
> mdio_read() calls and looking at the mii.c file we can see that calls to
> functions like mdio_read (and a lot of others) dont check return values.
>   
> So in light of this I think a better patch would be to not edit the 
> mii.c file at all and just make ch9200_mdio_read return 0 on     
> error.

Do you actually have one of these devices? If you do have, an even
better change would be to throwaway the mii code and swap to phylib
and an MDIO bus. You can probably follow smsc95xx.c.

	Andrew

Re: [PATCH 1/4] net: fix uninitialised access in mii_nway_restart()

[Qasim Ijaz]

On Fri, Apr 11, 2025 at 03:12:06AM +0200, Andrew Lunn wrote:
> On Thu, Apr 10, 2025 at 11:15:23PM +0100, Qasim Ijaz wrote:
> > On Tue, Mar 25, 2025 at 06:33:07AM -0700, Jakub Kicinski wrote:
> > > On Wed, 19 Mar 2025 11:21:53 +0000 Qasim Ijaz wrote:
> > > > --- a/drivers/net/mii.c
> > > > +++ b/drivers/net/mii.c
> > > > @@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii)
> > > >  
> > > >  	/* if autoneg is off, it's an error */
> > > >  	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
> > > > +	if (bmcr < 0)
> > > > +		return bmcr;
> > > >  
> > > >  	if (bmcr & BMCR_ANENABLE) {
> > > >  		bmcr |= BMCR_ANRESTART;
> > > 
> > > We error check just one mdio_read() but there's a whole bunch of them
> > > in this file. What's the expected behavior then? Are all of them buggy?
> > >
> >  
> > Hi Jakub
> >     
> > Apologies for my delayed response, I had another look at this and I
> > think my patch may be off a bit. You are correct that there are multiple
> > mdio_read() calls and looking at the mii.c file we can see that calls to
> > functions like mdio_read (and a lot of others) dont check return values.
> >   
> > So in light of this I think a better patch would be to not edit the 
> > mii.c file at all and just make ch9200_mdio_read return 0 on     
> > error.
> 
> Do you actually have one of these devices? If you do have, an even
> better change would be to throwaway the mii code and swap to phylib
> and an MDIO bus. You can probably follow smsc95xx.c.
> 

Hi Andrew,

Thanks for the suggestion. I don't have one of these devices at the moment.
If in the future if I do I will definitely explore the suggestion more.

Regards,
Qasim

> 	Andrew