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

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

[Qasim Ijaz]

On Tue, Feb 18, 2025 at 02:10:08AM +0100, Andrew Lunn wrote:
> On Tue, Feb 18, 2025 at 12:24:43AM +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.
> 
> What about get_mac_address()?
> 
> If you find a bug, it is a good idea to look around and see if there
> are any more instances of the same bug. I could be wrong, but it seems
> like get_mac_address() suffers from the same problem?

Thank you for the feedback Andrew. I checked get_mac_address() before
sending this patch and to me it looks like it does check the return value of
control_read(). It accumulates the return value of each control_read() call into 
rd_mac_len and then checks if it not equal to what is expected (ETH_ALEN which is 6),
I believe each call should return 2.

> 
> 	Andrew

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

[Andrew Lunn]

On Tue, Feb 18, 2025 at 12:19:57PM +0000, Qasim Ijaz wrote:
> On Tue, Feb 18, 2025 at 02:10:08AM +0100, Andrew Lunn wrote:
> > On Tue, Feb 18, 2025 at 12:24:43AM +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.
> > 
> > What about get_mac_address()?
> > 
> > If you find a bug, it is a good idea to look around and see if there
> > are any more instances of the same bug. I could be wrong, but it seems
> > like get_mac_address() suffers from the same problem?
> 
> Thank you for the feedback Andrew. I checked get_mac_address() before
> sending this patch and to me it looks like it does check the return value of
> control_read(). It accumulates the return value of each control_read() call into 
> rd_mac_len and then checks if it not equal to what is expected (ETH_ALEN which is 6),
> I believe each call should return 2.

It is unlikely a real device could trigger an issue, but a USB Rubber
Ducky might be able to. So the question is, are you interested in
protecting against malicious devices, or just making a static analyser
happy? Feel free to submit the patch as is.

	Andrew

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

[Qasim Ijaz]

On Wed, Feb 26, 2025 at 02:43:31PM +0100, Andrew Lunn wrote:
> On Tue, Feb 18, 2025 at 12:19:57PM +0000, Qasim Ijaz wrote:
> > On Tue, Feb 18, 2025 at 02:10:08AM +0100, Andrew Lunn wrote:
> > > On Tue, Feb 18, 2025 at 12:24:43AM +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.
> > > 
> > > What about get_mac_address()?
> > > 
> > > If you find a bug, it is a good idea to look around and see if there
> > > are any more instances of the same bug. I could be wrong, but it seems
> > > like get_mac_address() suffers from the same problem?
> > 
> > Thank you for the feedback Andrew. I checked get_mac_address() before
> > sending this patch and to me it looks like it does check the return value of
> > control_read(). It accumulates the return value of each control_read() call into 
> > rd_mac_len and then checks if it not equal to what is expected (ETH_ALEN which is 6),
> > I believe each call should return 2.
> 
> It is unlikely a real device could trigger an issue, but a USB Rubber
> Ducky might be able to. So the question is, are you interested in
> protecting against malicious devices, or just making a static analyser
> happy? Feel free to submit the patch as is.
> 
> 	Andrew

Hi Andrew,

Just following up on my patch regarding the uninitialized access fix in mii_nway_restart(). 
As I mentioned in my previous message, how about an approach similar to the patch for ch9200_mdio_read() 
for get_mac_address() where we immediately check the return value of each control_read() call and return 
an error if any call fails? This way we don't continue if failure occurs. If you're good with this approach, 
should I submit a patch v2?

Thanks,
Qasim
> 

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

[Andrew Lunn]

> Hi Andrew,
> 
> Just following up on my patch regarding the uninitialized access fix in mii_nway_restart(). 
> As I mentioned in my previous message, how about an approach similar to the patch for ch9200_mdio_read() 
> for get_mac_address() where we immediately check the return value of each control_read() call and return 
> an error if any call fails? This way we don't continue if failure occurs. If you're good with this approach, 
> should I submit a patch v2?

Yes, this would be good.

Thanks
	Andrew

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

[Qasim Ijaz]

On Wed, Feb 26, 2025 at 02:43:31PM +0100, Andrew Lunn wrote:
> On Tue, Feb 18, 2025 at 12:19:57PM +0000, Qasim Ijaz wrote:
> > On Tue, Feb 18, 2025 at 02:10:08AM +0100, Andrew Lunn wrote:
> > > On Tue, Feb 18, 2025 at 12:24:43AM +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.
> > > 
> > > What about get_mac_address()?
> > > 
> > > If you find a bug, it is a good idea to look around and see if there
> > > are any more instances of the same bug. I could be wrong, but it seems
> > > like get_mac_address() suffers from the same problem?
> > 
> > Thank you for the feedback Andrew. I checked get_mac_address() before
> > sending this patch and to me it looks like it does check the return value of
> > control_read(). It accumulates the return value of each control_read() call into 
> > rd_mac_len and then checks if it not equal to what is expected (ETH_ALEN which is 6),
> > I believe each call should return 2.
> 
> It is unlikely a real device could trigger an issue, but a USB Rubber
> Ducky might be able to. So the question is, are you interested in
> protecting against malicious devices, or just making a static analyser
> happy? Feel free to submit the patch as is.
> 
Hi Andrew,

How about an approach similar to the patch for ch9200_mdio_read(), where we immediately check the return value of 
each control_read() call in get_mac_address(), and if one fails we stop and return an error right away? 
That would ensure we don’t continue if an earlier call fails.

Let me know if you’d like me to submit a patch v2 if this sounds good.

Thanks, 
Qasim

> 	Andrew
> 

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

[Qasim Ijaz]

Hi Andrew,

Just following up on my patch from Feb 18 regarding the uninitialised access fix in mii_nway_restart(). Any further feedback would be appreciated.

Thanks,
Qasim

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

Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
Reported-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
Tested-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=3361c2d6f78a3e0892f9
Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
Cc: stable@vger.kernel.org
---
 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..e32d3c282dc1 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 != 2)
+		return ret < 0 ? ret : -EINVAL;
 
 	return (buff[0] | buff[1] << 8);
 }
-- 
2.39.5

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

[Andrew Lunn]

On Tue, Feb 18, 2025 at 12:24:43AM +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.

What about get_mac_address()?

If you find a bug, it is a good idea to look around and see if there
are any more instances of the same bug. I could be wrong, but it seems
like get_mac_address() suffers from the same problem?

	Andrew