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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Provider module unload causes use-after-free due to missing module and device lifecycle references in fwnode_pcs_get().
- [High] Returning -ENODEV and compacting the available_pcs array breaks probe deferral and destroys hardware port mappings.
- [Low] devm_fwnode_pcs_add_provider() unnecessarily allocates devres memory for NULL fwnodes.
- [Low] Redundant condition in fwnode_parse_pcsspec() leads to dead code.
--

--- Patch [3]: [PATCH net-next v11 03/12] net: pcs: implement Firmware node support for PCS driver ---
commit 9eb9adef70448b865a27066bb2a6ee2e49d74279
Author: Christian Marangi <[email protected]>

net: pcs: implement Firmware node support for PCS driver

Implement the foundation of Firmware node support for PCS driver. This adds
a Provider API where a PCS driver can expose multiple PCS with an xlate
function, and consumers can request them.

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

[ ... ]

> +struct fwnode_pcs_provider *
> +devm_fwnode_pcs_add_provider(struct device *dev, struct fwnode_handle *fwnode,
> +			     struct phylink_pcs *(*get)(struct fwnode_reference_args *pcsspec,
> +							void *data),
> +			     void *data)
> +{
> +	struct fwnode_pcs_provider **ptr, *pp;
> +
> +	ptr = devres_alloc(devm_fwnode_pcs_del, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pp = fwnode_pcs_add_provider(fwnode, get, data);
> +
> +	if (!IS_ERR(pp)) {

[Severity: Low]
Does this unintentionally allocate devres memory for NULL fwnodes?

If a driver calls this with a NULL fwnode, fwnode_pcs_add_provider() returns
0 (NULL). Since IS_ERR() does not catch NULL pointers, !IS_ERR(pp) evaluates
to true, resulting in a devres entry tracking a NULL provider.

> +		*ptr = pp;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return pp;
> +}
> +EXPORT_SYMBOL_GPL(devm_fwnode_pcs_add_provider);
> +
> +static int fwnode_parse_pcsspec(const struct fwnode_handle *fwnode,
> +				int index, const char *name,
> +				struct fwnode_reference_args *out_args)
> +{
> +	int ret;
> +
> +	if (!fwnode)
> +		return -EINVAL;
> +
> +	if (name) {
> +		index = fwnode_property_match_string(fwnode, "pcs-names",
> +						     name);
> +		if (index < 0)
> +			return index;
> +	}
> +
> +	ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> +						 "#pcs-cells", 0, index,
> +						 out_args);
> +	if (ret || (name && index < 0))

[Severity: Low]
Is this (name && index < 0) check unreachable?

Since the earlier block already returns if index < 0 when name is provided:

	if (name) {
		index = fwnode_property_match_string(fwnode, "pcs-names",
						     name);
		if (index < 0)
			return index;
	}

Could this redundant check be safely removed?

> +		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);
> +
> +	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);
> +			if (!IS_ERR(pcs))
> +				break;
> +		}
> +	}
> +	mutex_unlock(&fwnode_pcs_mutex);
> +
> +	return pcs;

[Severity: High]
Does this allow a use-after-free if the provider module is unloaded?

The phylink_pcs pointer is returned to the consumer here without acquiring
a module reference (like try_module_get) or establishing a device link
(like device_link_add).

If the administrator unbinds the PCS provider driver, it looks like
fwnode_pcs_del_provider() tears down the provider without revoking
consumers' dangling pointers, which could lead to a crash during subsequent
phylink operations.

> +}

[ ... ]

> +int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
> +			     struct phylink_pcs **available_pcs,
> +			     unsigned int num_pcs)
> +{
> +	unsigned int i, found = 0;
> +
> +	if (!available_pcs)
> +		return -EINVAL;
> +
> +	if (!fwnode_property_present(fwnode, "pcs-handle"))
> +		return -ENODEV;
> +
> +	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 continuing on -ENODEV break probe deferral and misalign the array?

If a MAC driver requests multiple PCS nodes and one has not yet probed,
fwnode_pcs_get() returns -ENODEV. By continuing without returning
-EPROBE_DEFER, the loop skips the unprobed PCS without incrementing the
found counter.

Could this cause subsequent valid PCS entries to be stored at the wrong
indices in available_pcs, leading to hardware being configured incorrectly?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3