| 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
| ||
|
Message-ID: <aLl0cbYv-fY-tPpI@pie>
Date: Thu, 4 Sep 2025 11:13:53 +0000
From: Yao Zi <ziyao@...root.org>
To: Simon Horman <horms@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
"Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
Jonas Karlman <jonas@...boo.se>, David Wu <david.wu@...k-chips.com>,
Chaoyi Chen <chaoyi.chen@...k-chips.com>, netdev@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH net] net: stmmac: dwmac-rk: Ensure clk_phy doesn't
contain invalid address
On Thu, Sep 04, 2025 at 11:34:43AM +0100, Simon Horman wrote:
> On Thu, Sep 04, 2025 at 03:12:24AM +0000, Yao Zi wrote:
> > We must set the clk_phy pointer to NULL to indicating it isn't available
> > if the optional phy clock couldn't be obtained. Otherwise the error code
> > returned by of_clk_get() could be wrongly taken as an address, causing
> > invalid pointer dereference when later clk_phy is passed to
> > clk_prepare_enable().
> >
> > Fixes: da114122b831 ("net: ethernet: stmmac: dwmac-rk: Make the clk_phy could be used for external phy")
> > Signed-off-by: Yao Zi <ziyao@...root.org>
> > ---
> > drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > On next-20250903, the fixed commit causes NULL pointer dereference on
> > Radxa E20C during probe of dwmac-rk, a typical dmesg looks like
> >
> > [ 0.273324] rk_gmac-dwmac ffbe0000.ethernet: IRQ eth_lpi not found
> > [ 0.273888] rk_gmac-dwmac ffbe0000.ethernet: IRQ sfty not found
> > [ 0.274520] rk_gmac-dwmac ffbe0000.ethernet: PTP uses main clock
> > [ 0.275226] rk_gmac-dwmac ffbe0000.ethernet: clock input or output? (output).
> > [ 0.275867] rk_gmac-dwmac ffbe0000.ethernet: Can not read property: tx_delay.
> > [ 0.276491] rk_gmac-dwmac ffbe0000.ethernet: set tx_delay to 0x30
> > [ 0.277026] rk_gmac-dwmac ffbe0000.ethernet: Can not read property: rx_delay.
> > [ 0.278086] rk_gmac-dwmac ffbe0000.ethernet: set rx_delay to 0x10
> > [ 0.278658] rk_gmac-dwmac ffbe0000.ethernet: integrated PHY? (no).
> > [ 0.279249] Unable to handle kernel paging request at virtual address fffffffffffffffe
> > [ 0.279948] Mem abort info:
> > [ 0.280195] ESR = 0x000000096000006
> > [ 0.280523] EC = 0x25: DABT (current EL), IL = 32 bits
> > [ 0.280989] SET = 0, FnV = 0
> > [ 0.281287] EA = 0, S1PTW = 0
> > [ 0.281574] FSC = 0x06: level 2 translation fault
> >
> > where the invalid address is just -ENOENT (-2).
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > index cf619a428664..26ec8ae662a6 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > @@ -1414,11 +1414,17 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat)
> > if (plat->phy_node) {
> > bsp_priv->clk_phy = of_clk_get(plat->phy_node, 0);
> > ret = PTR_ERR_OR_ZERO(bsp_priv->clk_phy);
> > - /* If it is not integrated_phy, clk_phy is optional */
> > + /*
> > + * If it is not integrated_phy, clk_phy is optional. But we must
> > + * set bsp_priv->clk_phy to NULL if clk_phy isn't proivded, or
> > + * the error code could be wrongly taken as an invalid pointer.
> > + */
> > if (bsp_priv->integrated_phy) {
> > if (ret)
> > return dev_err_probe(dev, ret, "Cannot get PHY clock\n");
> > clk_set_rate(bsp_priv->clk_phy, 50000000);
> > + } else if (ret) {
> > + bsp_priv->clk_phy = NULL;
> > }
> > }
>
> Thanks, and sorry for my early confusion about applying this patch.
>
> I agree that the bug you point out is addressed by this patch.
> Although I wonder if it is cleaner not to set bsp_priv->clk_phy
> unless there is no error, rather than setting it then resetting
> it if there is an error.
Yes, it sounds more natural to have a temporary variable storing result
of of_clk_get() and only assign it to clk_phy when the result is valid.
> More importantly, I wonder if there is another bug: does clk_set_rate need
> to be called in the case where there is no error and bsp_priv->integrated_phy
> is false?
In my understanding this may be intended, bsp_priv->integrated_phy is
only false when an external phy is used, and an external phy might
require arbitrary clock rates, thus it doesn't seem a good idea to me to
hardcode the clock rate in the driver.
I guess rate of clk_phy could also be set up with assigned-clock-rates
in devicetree. If so it may be reasonable to enable the clock only.
> So I am wondering if it makes sense to go with something like this.
> (Compile tested only!)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 266c53379236..a25816af2c37 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -1411,12 +1411,16 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat)
> }
>
> if (plat->phy_node) {
> - bsp_priv->clk_phy = of_clk_get(plat->phy_node, 0);
> - ret = PTR_ERR_OR_ZERO(bsp_priv->clk_phy);
> - /* If it is not integrated_phy, clk_phy is optional */
> - if (bsp_priv->integrated_phy) {
> - if (ret)
> + struct clk *clk_phy;
> +
> + clk_phy = of_clk_get(plat->phy_node, 0);
> + ret = PTR_ERR_OR_ZERO(clk_phy);
> + if (ret) {
> + /* If it is not integrated_phy, clk_phy is optional */
> + if (bsp_priv->integrated_phy)
> return dev_err_probe(dev, ret, "Cannot get PHY clock\n");
> + } else {
> + bsp_priv->clk_phy = clk_phy;
> clk_set_rate(bsp_priv->clk_phy, 50000000);
> }
> }
>
> Please note: if you send an updated patch (against net) please
> make sure you wait 24h before the original post.
>
> See: https://docs.kernel.org/process/maintainer-netdev.html
Thanks for the tip. While digging through the problematic commit for the
clk_phy's rate problem, I found others have discovered the problem[1]
and proposed some fixes (though there hasn't been a formal patch).
I should have read the original thread before sending this patch! Will
wait for some time and see whether the netdev maintainer prefers waiting
for original author's fix or taking mine.
Best regards,
Yao Zi
[1]: https://lore.kernel.org/netdev/a30a8c97-6b96-45ba-bad7-8a40401babc2@samsung.com/
Powered by blists - more mailing lists