* [PATCH] net: phy: dp83869: fix memory corruption when enabling fiber
@ 2024-10-01 7:57 Ingo van Lil
2024-10-01 10:40 ` Sverdlin, Alexander
2024-10-02 15:54 ` Andrew Lunn
0 siblings, 2 replies; 6+ messages in thread
From: Ingo van Lil @ 2024-10-01 7:57 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: Dan Murphy, Ingo van Lil
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: phy: dp83869: fix memory corruption when enabling fiber
2024-10-01 7:57 [PATCH] net: phy: dp83869: fix memory corruption when enabling fiber Ingo van Lil
@ 2024-10-01 10:40 ` Sverdlin, Alexander
2024-10-01 13:31 ` Ingo van Lil
2024-10-02 15:54 ` Andrew Lunn
1 sibling, 1 reply; 6+ messages in thread
From: Sverdlin, Alexander @ 2024-10-01 10:40 UTC (permalink / raw)
To: inguin, linux-kernel, netdev; +Cc: dmurphy
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: phy: dp83869: fix memory corruption when enabling fiber
2024-10-01 10:40 ` Sverdlin, Alexander
@ 2024-10-01 13:31 ` Ingo van Lil
2024-10-01 13:45 ` Sverdlin, Alexander
0 siblings, 1 reply; 6+ messages in thread
From: Ingo van Lil @ 2024-10-01 13:31 UTC (permalink / raw)
To: Sverdlin, Alexander, linux-kernel, netdev; +Cc: dmurphy
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: dp83869: fix memory corruption when enabling fiber
2024-10-01 13:31 ` Ingo van Lil
@ 2024-10-01 13:45 ` Sverdlin, Alexander
2024-10-02 15:53 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Sverdlin, Alexander @ 2024-10-01 13:45 UTC (permalink / raw)
To: inguin, linux-kernel, netdev; +Cc: dmurphy
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: dp83869: fix memory corruption when enabling fiber
2024-10-01 13:45 ` Sverdlin, Alexander
@ 2024-10-02 15:53 ` Andrew Lunn
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Lunn @ 2024-10-02 15:53 UTC (permalink / raw)
To: Sverdlin, Alexander; +Cc: inguin, linux-kernel, netdev, dmurphy
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: dp83869: fix memory corruption when enabling fiber
2024-10-01 7:57 [PATCH] net: phy: dp83869: fix memory corruption when enabling fiber Ingo van Lil
2024-10-01 10:40 ` Sverdlin, Alexander
@ 2024-10-02 15:54 ` Andrew Lunn
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Lunn @ 2024-10-02 15:54 UTC (permalink / raw)
To: Ingo van Lil; +Cc: linux-kernel, netdev, Dan Murphy
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-10-02 15:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-01 7:57 [PATCH] net: phy: dp83869: fix memory corruption when enabling fiber Ingo van Lil
2024-10-01 10:40 ` Sverdlin, Alexander
2024-10-01 13:31 ` Ingo van Lil
2024-10-01 13:45 ` Sverdlin, Alexander
2024-10-02 15:53 ` Andrew Lunn
2024-10-02 15:54 ` Andrew Lunn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®