Re: [PATCH net-next v18 02/12] net: phylink: introduce internal phylink PCS handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: phylink: UAF of phylink struct in phylink_pcs_change() during concurrent teardown
- [Medium] net: phylink: missing validation failure when a required PCS is not found
--

commit 7ece69b824da0eb34269e858e3ce098347af5b79
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: introduce internal phylink PCS handling

This commit introduces internal handling of PCS for phylink by migrating the
selection logic to use a list of available PCS attached to the phylink
instance. It updates validation and configuration functions to search this
list if a dedicated PCS is required for the requested interface mode.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a98e62fa4bd26..1156e6c61a08b 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -530,6 +549,21 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
>  		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
>  		if (IS_ERR(pcs))
>  			return PTR_ERR(pcs);
> +	/*
> +	 * Find a PCS in available PCS list for the requested interface.
> +	 *
> +	 * Skip searching if the MAC doesn't require a dedicated PCS for
> +	 * the requested interface.
> +	 */
> +	} else if (test_bit(state->interface, pl->config->pcs_interfaces)) {
> +		struct phylink_pcs *tmp;
> +
> +		list_for_each_entry(tmp, &pl->pcs_list, list) {
> +			if (!phylink_validate_pcs_interface(tmp, state->interface)) {
> +				pcs = tmp;
> +				break;
> +			}
> +		}
>  	}
>  
>  	if (pcs) {

[Severity: Medium]
If the MAC requires a dedicated PCS for the requested interface but no PCS in
pl->pcs_list supports it, pcs remains NULL. Since there is no check for !pcs
here, the function will bypass the if (pcs) validation block and incorrectly
return 0 (success).

Will this cause unsupported link modes to be falsely reported as supported
during phylink_validate()? It appears that later during
phylink_major_config(), the exact same lookup correctly checks for !pcs and
fatally aborts the link configuration. Should this return an error like
-EINVAL when a required PCS is not found?

[ ... ]
> @@ -2076,11 +2215,20 @@ EXPORT_SYMBOL_GPL(phylink_create);
>   */
>  void phylink_destroy(struct phylink *pl)
>  {
> +	struct phylink_pcs *pcs, *tmp;
> +
>  	sfp_bus_del_upstream(pl->sfp_bus);
>  	if (pl->link_gpio)
>  		gpiod_put(pl->link_gpio);
>  
>  	cancel_work_sync(&pl->resolve);
> +
> +	/* Remove every PCS from phylink PCS list */
> +	list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list) {
> +		pcs->phylink = NULL;
> +		list_del(&pcs->list);
> +	}
> +
>  	kfree(pl);
>  }
>  EXPORT_SYMBOL_GPL(phylink_destroy);

[Severity: High]
Does this teardown path properly synchronize with concurrent PCS events?

This path clears pcs->phylink and frees pl unconditionally, without an RCU
grace period or synchronization against concurrent execution. If
phylink_pcs_change() is called by an external PCS driver (e.g. from an
interrupt handler) and is delayed, a concurrent MAC driver unbind could
invoke phylink_destroy().

Because the teardown frees pl without waiting for pending PCS interrupts to
finish, this might leave readers in phylink_pcs_change() vulnerable to a
use-after-free.

[ ... ]
> @@ -2546,8 +2694,15 @@ void phylink_pcs_change(struct phylink_pcs *pcs, bool up)
>  {
>  	struct phylink *pl = pcs->phylink;
>  
> -	if (pl)
> -		phylink_link_changed(pl, up, "pcs");
> +	/*
> +	 * Ignore PCS link state change if the PCS is not
> +	 * attached to a phylink instance or the phylink
> +	 * instance is not currently using this PCS.
> +	 */
> +	if (!pl || READ_ONCE(pl->pcs) != pcs)
> +		return;
> +
> +	phylink_link_changed(pl, up, "pcs");
>  }
>  EXPORT_SYMBOL_GPL(phylink_pcs_change);

[Severity: High]
Can pl be a dangling pointer when this READ_ONCE() is executed?

As noted above in phylink_destroy(), pl could be freed if teardown happens
concurrently after pl is read from pcs->phylink but before this READ_ONCE()
completes, resulting in a use-after-free.

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