linux-kernel - 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: <c26909742e1f2bbe8f96699c1bbd54c2eada42ce.camel@siemens.com>
Date: Tue, 1 Oct 2024 13:45:11 +0000
From: "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>
To: "inguin@....de" <inguin@....de>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
CC: "dmurphy@...com" <dmurphy@...com>
Subject: Re: [PATCH] net: phy: dp83869: fix memory corruption when enabling
 fiber

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.

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com