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

[Qasim Ijaz <qasdev00@gmail.com>]

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
>