Re: [PATCH net-next v9 04/12] net: pcs: implement Firmware node support for PCS driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The PCS provider API lacks module reference counting or device linking, creating a use-after-free vulnerability.
- [High] Ignoring missing PCS providers breaks probe deferral and corrupts the mapping of the retrieved PCS array via unintended array squashing.
- [High] Potential AB-BA deadlock due to calling provider callback while holding global fwnode_pcs_mutex.
- [Low] API Misuse: Passing -1 to the unsigned int nargs parameter.
--

--- Patch [4]: [PATCH net-next v9 04/12] net: pcs: implement Firmware node support for PCS driver ---
commit 9d0c6df31924499c98bee1b3b6194d084ae4c002
Author: Christian Marangi <ansuelsmth@gmail.com>

net: pcs: implement Firmware node support for PCS driver

Implement the foundation of Firmware node support for PCS driver.
To support this, implement a simple Provider API where a PCS driver can
expose multiple PCS with an xlate .get function.

> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> new file mode 100644
> index 0000000000000..0cc4daf7beeae
> --- /dev/null
> +++ b/drivers/net/pcs/pcs.c
> @@ -0,0 +1,212 @@

[ ... ]

> +int fwnode_pcs_add_provider(struct fwnode_handle *fwnode,
> +			    struct phylink_pcs *(*get)(struct fwnode_reference_args *pcsspec,
> +						       void *data),
> +			    void *data)
> +{

[Severity: High]
Does this API need a way to track module ownership or establish device links?
Since providers don't pass their module owner (e.g., THIS_MODULE) or a device
pointer during registration, consumers can't hold a reference to the provider
module. If a provider module is unloaded, could it leave consumers with
dangling phylink_pcs pointers resulting in a use-after-free?

> +	struct fwnode_pcs_provider *pp;

[ ... ]

> +static int fwnode_parse_pcsspec(const struct fwnode_handle *fwnode,
> +				int index, const char *name,
> +				struct fwnode_reference_args *out_args)
> +{

[ ... ]

> +	ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> +						 "#pcs-cells",
> +						 -1, index, out_args);

[Severity: Low]
Is it intentional to pass -1 to the nargs parameter of
fwnode_property_get_reference_args()? Since nargs is an unsigned int, this
will be implicitly cast to UINT_MAX.

> +	if (ret || (name && index < 0))
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static struct phylink_pcs *
> +fwnode_pcs_get_from_pcsspec(struct fwnode_reference_args *pcsspec)
> +{
> +	struct fwnode_pcs_provider *provider;
> +	struct phylink_pcs *pcs = ERR_PTR(-ENODEV);

[Severity: High]
Should this default to -EPROBE_DEFER instead of -ENODEV? If a provider hasn't
probed yet, returning -ENODEV will prevent the consumer from correctly
deferring its probe.

> +
> +	if (!pcsspec)
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&fwnode_pcs_mutex);
> +	list_for_each_entry(provider, &fwnode_pcs_providers, link) {
> +		if (provider->fwnode == pcsspec->fwnode) {
> +			pcs = provider->get(pcsspec, provider->data);

[Severity: High]
Can calling the provider's get() callback while holding the global
fwnode_pcs_mutex cause an AB-BA deadlock?

If an advanced provider needs to acquire a driver-local lock in its get()
callback, it creates a lock order of fwnode_pcs_mutex -> local_lock.

During provider removal, the driver might acquire local_lock before calling
fwnode_pcs_del_provider(), which then tries to acquire fwnode_pcs_mutex.

> +			if (!IS_ERR(pcs))
> +				break;
> +		}
> +	}
> +	mutex_unlock(&fwnode_pcs_mutex);
> +
> +	return pcs;
> +}
> +
> +static struct phylink_pcs *__fwnode_pcs_get(struct fwnode_handle *fwnode,
> +					    unsigned int index, const char *con_id)
> +{
> +	struct fwnode_reference_args pcsspec;
> +	struct phylink_pcs *pcs;
> +	int ret;
> +
> +	ret = fwnode_parse_pcsspec(fwnode, index, con_id, &pcsspec);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	pcs = fwnode_pcs_get_from_pcsspec(&pcsspec);
> +	fwnode_handle_put(pcsspec.fwnode);
> +
> +	return pcs;

[Severity: High]
Since this returns a bare pointer to the consumer without taking a reference
on the provider module, and there is no corresponding put() API, what prevents
the provider from freeing the PCS memory while the consumer is still using it?

> +}

[ ... ]

> +unsigned int fwnode_phylink_pcs_count(struct fwnode_handle *fwnode)
> +{
> +	struct fwnode_reference_args out_args;
> +	int index = 0;
> +	int ret;
> +
> +	while (true) {
> +		ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> +							 "#pcs-cells",
> +							 -1, index, &out_args);

[Severity: Low]
Is this another location where we are passing -1 to an unsigned int parameter?

> +		/* We expect to reach an -ENOENT error while counting */
> +		if (ret)
> +			break;

[ ... ]

> +int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
> +			     struct phylink_pcs **available_pcs,
> +			     unsigned int num_pcs)
> +{
> +	unsigned int i, found = 0;

[ ... ]

> +	for (i = 0; i < num_pcs; i++) {
> +		struct phylink_pcs *pcs;
> +
> +		pcs = fwnode_pcs_get(fwnode, i);
> +		if (IS_ERR(pcs)) {
> +			/* Exit early if no PCS remain.*/
> +			if (PTR_ERR(pcs) == -ENOENT)
> +				break;
> +
> +			/*
> +			 * Ignore -ENODEV error for PCS that still
> +			 * needs to probe.
> +			 */
> +			if (PTR_ERR(pcs) == -ENODEV)
> +				continue;

[Severity: High]
Does ignoring -ENODEV and continuing without incrementing the "found" index
corrupt the hardware mapping for the MAC driver?

By not storing a placeholder or deferring probe, any subsequently probed PCS
will be shifted into the wrong index in the available_pcs array. If the MAC
driver expects index i to map to physical lane i, won't this result in
incorrect routing?

> +
> +			return PTR_ERR(pcs);
> +		}
> +
> +		available_pcs[found] = pcs;
> +		found++;
> +	}
> +
> +	return found;
> +}

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