[v1] net: phy: dp83869: fix memory corruption when enabling fiber

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

Posted by Ingo van Lil 1 month, 4 weeks ago

When configuring the fiber port, the DP83869 PHY driver incorrectly
calls linkmode_set_bit() with a bit mask (1 << 10) rather than a bit
number (10). This corrupts some other memory location -- in case of
arm64 the priv pointer in the same structure.

Signed-off-by: Ingo van Lil <inguin@gmx.de>
---
 drivers/net/phy/dp83869.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);

 	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
 		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
--
2.46.1

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

Posted by Andrew Lunn 1 month, 3 weeks ago

On Tue, Oct 01, 2024 at 09:57:33AM +0200, Ingo van Lil wrote:
> When configuring the fiber port, the DP83869 PHY driver incorrectly
> calls linkmode_set_bit() with a bit mask (1 << 10) rather than a bit
> number (10). This corrupts some other memory location -- in case of
> arm64 the priv pointer in the same structure.

Please add a Fixes: tag, and base on net.

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html


    Andrew

---
pw-bot: cr

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

Posted by Sverdlin, Alexander 1 month, 4 weeks ago

Hi Ingo!

On Tue, 2024-10-01 at 09:57 +0200, Ingo van Lil wrote:
> When configuring the fiber port, the DP83869 PHY driver incorrectly
> calls linkmode_set_bit() with a bit mask (1 << 10) rather than a bit
> number (10). This corrupts some other memory location -- in case of
> arm64 the priv pointer in the same structure.
> 
> Signed-off-by: Ingo van Lil <inguin@gmx.de>
> ---
>  drivers/net/phy/dp83869.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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?

If we consider the following code at the very end of the same function?

        /* Update advertising from supported */
        linkmode_or(phydev->advertising, phydev->advertising,
                    phydev->supported);

> 
>  	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
>  		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

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

Posted by Ingo van Lil 1 month, 3 weeks ago

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.

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?

Regards,
Ingo

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

Posted by Sverdlin, Alexander 1 month, 3 weeks ago

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

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

Posted by Andrew Lunn 1 month, 3 weeks ago

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