Re: [PATCH net-next v20 08/15] net: phylink: support PCS provider release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential
issues to consider:
- [Medium] net: phylink: unchecked validation in
  phylink_calculate_capabilities()
- [Medium] net: phylink: pcs_provider_notify() clobbers mode and media
  linkmodes
- [Medium] net: phylink: duplicate phylink_pcs_disable() in
  phylink_del_pcs()
- [Medium] net: phylink: force_major_config in phylink_del_pcs() runs
  when disabled
- [Medium] net: phylink: race in phylink_create() after registering
  fwnode_pcs_nb
- [Medium] net: phylink: missing !pcs check in
  phylink_validate_mac_and_pcs()
- [Medium] net: phylink: UAF of phylink_pcs in phylink_destroy() and
  phylink_create()
--

commit 9f425957f6e59d4881b3a8c251eeacc60b88a666
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: support PCS provider release

This commit adds a blocking notifier chain for fwnode PCS provider
removal so phylink instances can detach removed PCS instances, refresh
supported interfaces and capabilities, and trigger link resolution.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 46b2aba336bd1..e807fcccb5de9 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -722,6 +725,8 @@ static void phylink_calculate_capabilities(struct phylink *pl)
>  {
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(disabled_advertising);
>  
> +	lockdep_assert_held(&pl->state_mutex);
> +
>  	/* Get user-disabled advertising modes */
>  	linkmode_xor(disabled_advertising, pl->advertising,
>  		     pl->link_config.advertising);
> @@ -729,7 +734,7 @@ static void phylink_calculate_capabilities(struct phylink *pl)
>  	/* Calculate the full supported advertisement */
>  	linkmode_fill(pl->supported);
>  	linkmode_copy(pl->link_config.advertising, pl->supported);
> -	phylink_validate(pl, pl->supported, &pl->link_config);
> +	__phylink_validate(pl, pl->supported, &pl->link_config);

[Severity: Medium]
Does phylink_calculate_capabilities() leave pl->supported and
pl->advertising filled with all ones when __phylink_validate() fails?

When pcs_provider_notify() removes the PCS that provided
pl->link_config.interface and rebuilds pl->supported_interfaces,
pl->link_config.interface still holds the removed interface mode.
__phylink_validate() then returns -EINVAL at the test_bit() check before
clearing or masking pl->supported or pl->link_config.advertising:

drivers/net/phy/phylink.c:__phylink_validate() {
	...
	if (!test_bit(state->interface, interfaces))
		return -EINVAL;
}

Because the return value is ignored after linkmode_fill(pl->supported),
every ethtool linkmode bit remains set in pl->supported, pl->advertising,
and (except for previously disabled bits) pl->link_config.advertising.

[ ... ]

> @@ -2066,11 +2084,91 @@ static int phylink_fill_available_pcs(struct phylink *pl,
>  	}
>  
>  out:
> +	mutex_unlock(&pl->state_mutex);
> +
>  	kfree(pcss);
>  
>  	return ret;
>  }
>  
> +static void phylink_del_pcs(struct phylink *pl, struct phylink_pcs *pcs)
> +{
> +	lockdep_assert_held(&pl->state_mutex);
> +
> +	rcu_assign_pointer(pcs->phylink, NULL);
> +	synchronize_rcu();
> +	list_del(&pcs->list);
> +
> +	/*
> +	 * Check if we are removing the PCS currently
> +	 * in use by this phylink instance. If this is the case,
> +	 * tear down the link, force phylink resolve to reconfigure the
> +	 * interface mode, disable the current PCS and set the
> +	 * phylink PCS to NULL.
> +	 */
> +	if (pl->pcs == pcs) {
> +		if (pl->old_link_state) {
> +			phylink_link_down(pl);
> +			pl->old_link_state = false;
> +		}
> +		if (pl->cfg_link_an_mode == MLO_AN_INBAND)
> +			timer_delete_sync(&pl->link_poll);
> +		phylink_pcs_disable(pl->pcs);

[Severity: Medium]
Can phylink_del_pcs() call phylink_pcs_disable(pl->pcs) a second time
when the phylink instance is already stopped?

phylink_stop() sets pl->pcs_state to PCS_STATE_DOWN and calls
phylink_pcs_disable(pl->pcs) while leaving pl->pcs non-NULL.  If the PCS
provider is removed while stopped, phylink_del_pcs() calls
phylink_pcs_disable(pl->pcs) again without an intervening enable,
breaking PCS drivers that refcount enable and disable calls such as
mxl862xx_pcs_disable().  Should this check pl->pcs_state !=
PCS_STATE_DOWN before disabling the PCS?

> +
> +		pl->force_major_config = true;
> +		WRITE_ONCE(pl->pcs, NULL);
> +	}
> +}

[Severity: Medium]
Should pl->force_major_config be set when pl->phylink_disable_state is
non-zero?

In phylink_resolve(), pl->force_major_config bypasses the mac_config gate
that normally suppresses reconfiguration when pl->phylink_disable_state
is non-zero:

drivers/net/phy/phylink.c:phylink_resolve() {
	...
	if ((mac_config && link_state.interface != pl->link_config.interface) ||
	    pl->force_major_config) {
		...
		phylink_major_config(pl, false, &link_state);
		pl->link_config.interface = link_state.interface;
		pl->force_major_config = false;
	}
}

If phylink_stop() runs phylink_run_resolve_and_disable() after
timer_delete_sync(&pl->link_poll) while pl->force_major_config is set,
phylink_major_config() reconfigures the stopping link and calls
phylink_pcs_poll_start(), which can re-arm pl->link_poll after it was
deleted.  If the interface was already stopped when the PCS was removed,
pl->force_major_config stays set until the next phylink_start(), causing
phylink_enable_and_run_resolve() to run a duplicate phylink_major_config()
right after phylink_mac_initial_config().

> +
> +static int pcs_provider_notify(struct notifier_block *self,
> +			       unsigned long val, void *data)
> +{
> +	struct phylink *pl = container_of(self, struct phylink, fwnode_pcs_nb);
> +	struct fwnode_pcs_provider *pp = data;
> +	struct phylink_pcs *pcs, *tmp;
> +	bool resolve = false;
> +
> +	rtnl_lock();
> +	mutex_lock(&pl->state_mutex);
> +
> +	/*
> +	 * Loop all the PCS for phylink instance and check if
> +	 * this notification is relevant for some of them.
> +	 */
> +	list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list) {
> +		if (!fwnode_pcs_matches_provider(pp, pl->fwnode, pcs))
> +			continue;
> +
> +		phylink_del_pcs(pl, pcs);
> +		resolve = true;
> +	}
> +
> +	/* Exit early if nothing has changed */
> +	if (!resolve) {
> +		mutex_unlock(&pl->state_mutex);
> +		rtnl_unlock();
> +		return NOTIFY_DONE;
> +	}
> +
> +	/* Refresh supported interfaces */
> +	phy_interface_copy(pl->supported_interfaces,
> +			   pl->config->supported_interfaces);
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		phy_interface_or(pl->supported_interfaces,
> +				 pl->supported_interfaces,
> +				 pcs->supported_interfaces);

[Severity: Medium]
When a PCS is removed and pl->supported_interfaces is reset from
pl->config->supported_interfaces, can __phylink_validate() still treat an
interface in pl->config->pcs_interfaces as supported without a PCS?

Drivers populate pl->config->supported_interfaces with all MAC-supported
interfaces and mark those requiring a dedicated PCS in
pl->config->pcs_interfaces.  When no matching PCS remains in
pl->pcs_list, phylink_validate_mac_and_pcs() leaves pcs as NULL without
returning an error and validates against MAC capabilities alone:

drivers/net/phy/phylink.c:phylink_validate_mac_and_pcs() {
	...
	} 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) {
	...
}

Should phylink_validate_mac_and_pcs() return -EINVAL when !pcs in the
pl->config->pcs_interfaces branch, matching phylink_major_config()?

> +
> +	/* Recalculate capabilities */
> +	phylink_calculate_capabilities(pl);

[Severity: Medium]
Does calling phylink_calculate_capabilities() from pcs_provider_notify()
clobber mode- and media-specific masks in pl->supported and
pl->link_config.advertising?

After phylink_create() runs phylink_calculate_capabilities(),
phylink_parse_mode() (for MLO_AN_INBAND), phylink_parse_fixedlink(),
phylink_bringup_phy(), or SFP insertion restrict pl->supported and
pl->link_config.advertising without updating pl->advertising.  When
pcs_provider_notify() later calls phylink_calculate_capabilities(),
linkmode_xor(disabled_advertising, pl->advertising,
pl->link_config.advertising) treats those mode, PHY, or SFP restrictions
as user-disabled bits, and linkmode_fill(pl->supported) followed by
__phylink_validate() overwrites pl->supported and pl->advertising with
unconstrained MAC/PCS capabilities.

[ ... ]

> @@ -2281,10 +2379,18 @@ struct phylink *phylink_create(struct phylink_config *config,
>  	__set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
>  	timer_setup(&pl->link_poll, phylink_fixed_poll, 0);
>  
> +	/* First register notifier for hotplug PCS events */
> +	if (!phy_interface_empty(config->pcs_interfaces)) {
> +		pl->fwnode_pcs_nb.notifier_call = pcs_provider_notify;
> +		register_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);
> +	}
> +
>  	/* Fill the PCS list with available PCS from phylink config */
>  	ret = phylink_fill_available_pcs(pl, config);
>  	if (ret < 0)
> -		goto free_pl;
> +		goto unregister_pcs_notify;
> +
> +	mutex_lock(&pl->state_mutex);
>  
>  	/* Link available PCS to phylink */
>  	list_for_each_entry(pcs, &pl->pcs_list, list)
> @@ -2301,9 +2407,11 @@ struct phylink *phylink_create(struct phylink_config *config,
>  
>  	phylink_calculate_capabilities(pl);
>  
> +	mutex_unlock(&pl->state_mutex);
> +
>  	ret = phylink_parse_mode(pl, fwnode);
>  	if (ret < 0)
> -		goto unlink_pcs_list;
> +		goto unregister_pcs_notify;

[Severity: Medium]
Can pcs_provider_notify() race with phylink_create() because
pl->fwnode_pcs_nb is registered before initialization finishes and
pl->state_mutex is dropped between steps?

phylink_create() is called without rtnl_lock() held, so only
pl->state_mutex serializes it against pcs_provider_notify():

CPU0                                  CPU1
phylink_create()
  register_fwnode_pcs_notifier()
  phylink_fill_available_pcs()
    mutex_lock(&pl->state_mutex)
    list_add_tail(&pcs->list, ...)
    mutex_unlock(&pl->state_mutex)
  ...
  mutex_lock(&pl->state_mutex)
  phylink_calculate_capabilities()
  mutex_unlock(&pl->state_mutex)
                                      fwnode_pcs_del_provider()
                                        pcs_provider_notify()
                                          mutex_lock(&pl->state_mutex)
                                          phylink_calculate_capabilities()
  phylink_parse_mode()
    linkmode_zero(pl->supported)  <-- data race on pl->supported /
                                      pl->link_config.advertising

pcs_provider_notify() can also run between phylink_fill_available_pcs()
unlocking pl->state_mutex and phylink_create() re-locking it before
pcs->phylink and pl->supported_interfaces are initialized.

[ ... ]

> @@ -2322,14 +2430,18 @@ struct phylink *phylink_create(struct phylink_config *config,
>  release_link_gpio:
>  	if (pl->link_gpio)
>  		gpiod_put(pl->link_gpio);
> -unlink_pcs_list:
> +unregister_pcs_notify:
> +	if (pl->fwnode_pcs_nb.notifier_call)
> +		unregister_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);
>  	list_for_each_entry(pcs, &pl->pcs_list, list)
>  		rcu_assign_pointer(pcs->phylink, NULL);
> -
>  	synchronize_rcu();
> -
> +	/* PCS notifier might queue a resolve, cancel it */
> +	cancel_work_sync(&pl->resolve);
> +	mutex_lock(&pl->state_mutex);
>  	list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list)
>  		list_del(&pcs->list);
> +	mutex_unlock(&pl->state_mutex);
>  free_pl:
>  	fwnode_handle_put(pl->fwnode);
>  	kfree(pl);
> @@ -2355,6 +2467,10 @@ void phylink_destroy(struct phylink *pl)
>  	if (pl->link_gpio)
>  		gpiod_put(pl->link_gpio);
>  
> +	/* Unregister notifier for late PCS attach */
> +	if (pl->fwnode_pcs_nb.notifier_call)
> +		unregister_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);
> +
>  	/* Remove phylink instance from every PCS */
>  	list_for_each_entry(pcs, &pl->pcs_list, list)
>  		rcu_assign_pointer(pcs->phylink, NULL);
> @@ -2367,10 +2483,14 @@ void phylink_destroy(struct phylink *pl)
>  
>  	cancel_work_sync(&pl->resolve);
>  
> +	mutex_lock(&pl->state_mutex);
> +
>  	/* Remove every PCS from phylink PCS list */
>  	list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list)
>  		list_del(&pcs->list);
>  
> +	mutex_unlock(&pl->state_mutex);

[Severity: Medium]
Can unregistering pl->fwnode_pcs_nb before detaching pl->pcs_list in
phylink_destroy() and the phylink_create() error path cause a
use-after-free on struct phylink_pcs?

fwnode_pcs_del_provider() relies on fwnode_pcs_notify_list to detach all
phylink consumers before returning to the PCS provider driver, which then
frees its struct phylink_pcs:

CPU0                                  CPU1
phylink_destroy()
  unregister_fwnode_pcs_notifier()
                                      pcs_driver_remove()
                                        fwnode_pcs_del_provider()
                                          blocking_notifier_call_chain()
                                            /* pl already unregistered */
                                        kfree(pcs)
  list_for_each_entry(pcs, ...)
    rcu_assign_pointer(pcs->phylink, NULL)  <-- use-after-free
  synchronize_rcu()
  ...
  list_del(&pcs->list)                      <-- use-after-free

Unregistering the notifier after removing all PCS entries from
pl->pcs_list and clearing pl->pcs would prevent fwnode_pcs_del_provider()
from returning while phylink still holds pointers to those PCS instances.

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