netdev - Re: [PATCH] net: phy: dp83869: fix memory corruption when enabling fiber

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2ac17718-9bd8-4bc4-8c80-0afff99b1ddc@lunn.ch>
Date: Wed, 2 Oct 2024 17:53:09 +0200
From: Andrew Lunn <andrew@...n.ch>
To: "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>
Cc: "inguin@....de" <inguin@....de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"dmurphy@...com" <dmurphy@...com>
Subject: Re: [PATCH] net: phy: dp83869: fix memory corruption when enabling
 fiber

On Tue, Oct 01, 2024 at 01:45:11PM +0000, Sverdlin, Alexander wrote:
> Hi Ingo!
> 
> On Tue, 2024-10-01 at 15:31 +0200, Ingo van Lil wrote:
> > On 10/1/24 12:40, Sverdlin, Alexander wrote:
> > 
> > > > diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
> > > > index d7aaefb5226b..9c5ac5d6a9fd 100644
> > > > --- a/drivers/net/phy/dp83869.c
> > > > +++ b/drivers/net/phy/dp83869.c
> > > > @@ -645,7 +645,7 @@ static int dp83869_configure_fiber(struct phy_device *phydev,
> > > >    		     phydev->supported);
> > > > 
> > > >    	linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported);
> > > > -	linkmode_set_bit(ADVERTISED_FIBRE, phydev->advertising);
> > > > +	linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->advertising);
> > > 
> > > Are you sure this linkmode_set_bit() is required at all?
> > 
> > You're right, it's probably not required. I just tracked a weird bug
> > down to this clear mistake and wanted to change as little as possible.
> 
> As little as possible would be not to add yet another bit set.
> Obviously it has been working (if it was at all) without a proper write,
> but dispite the incorrect write.
> 
> > The logic of the function seems a bit odd to me: At the beginning,
> > advertising is ANDed with supported, and at the end it's ORed again.
> > Inside the function they are mostly manipulated together.
> > 
> > Couldn't that be replaced with a simple "phydev->advertising =
> > phydev->supported;" at the end?
> 
> Yes, the function looks strange.
> But as this is for -stable, maybe complete rework is undesired.
> IMO, just delete the bogus write.

+1 KISS for stable.

For net-next, as far as i can see, dp83869->mode is fixed at probe
time. As a lot of the code here should be moved into .get_features.

	Andrew