Re: [PATCH net-next v20 02/15] net: phylink: make PCS phylink instance access RCU protected

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] net: phylink: NULL deref and stale pointer in
  phylink_major_config()
- [Low] net: phylink: missing Fixes trailer for commit 24699cc1ff3e
--

commit 85080154c47d10a2333a2099ea90e555d71ad4e7
Author: Christian Marangi <ansuelsmth@gmail.com>

> net: phylink: make PCS phylink instance access RCU protected
>
> In the context of phylink_pcs_change(), phylink instance is dereferenced
> from the pcs pointer present in the phylink_pcs struct.
>
> There is currently a fragility where this pcs pointer is never set NULL in
> phylink_destroy() and would reference invalid memory after phylink instance
> is freed.
>
> In this scenario, if a PCS driver calls phylink_pcs_change(), invalid
> memory can be dereferenced and phylink_link_changed() is called on freed
> memory.
>
> To address this, make the phylink instance saved in phylink_pcs struct RCU
> protected and access it under RCU lock.
>
> Also correctly reset the phylink instance pointer on phylink_destroy() and
> syncronyze RCU to catch an execution of phylink_pcs_change().
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

[Severity: Low]
Since this fixes a use-after-free in phylink_pcs_change(), should it
include a Fixes tag for 24699cc1ff3e ("net: phylink: add support for PCS
link change notifications")?

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 6c339b25926cc..51c147eb8c2b6 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1300,10 +1301,10 @@ static void phylink_major_config(struct phylink *pl, bool restart,
>  		phylink_pcs_disable(pl->pcs);
>  
>  		if (pl->pcs)
> -			pl->pcs->phylink = NULL;
> +			rcu_assign_pointer(pcs->phylink, NULL);
>  
>  		if (pcs)
> -			pcs->phylink = pl;
> +			rcu_assign_pointer(pcs->phylink, pl);
>  
>  		pl->pcs = pcs;
>  	}

[Severity: High]
Should this clear pl->pcs->phylink instead of pcs->phylink?

When switching from an active PCS to an interface mode where pcs is NULL,
dereferencing pcs->phylink causes a NULL pointer dereference, regressing
0e4d7df2f3b2 ("net: phylink: fix NULL pointer deref in
phylink_major_config()").  When switching between two non-NULL PCS
instances, the old pl->pcs->phylink is never cleared, leaving a dangling
pointer to pl after phylink_destroy() frees it.

This regression is fixed later in the series by commit "net: phylink:
introduce internal phylink PCS handling".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114104426.235620-1-ansuelsmth@gmail.com?part=2