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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Lack of lifetime management between PCS consumer and provider allows Use-After-Free vulnerabilities.
- [High] Missing PCS providers are silently ignored instead of triggering probe deferral (-EPROBE_DEFER), breaking MAC linking and array indexing.
- [Medium] fwnode_pcs_get_from_pcsspec() drops error codes from the provider's xlate function and incorrectly continues searching the list.
--

--- Patch [3]: [PATCH v14 03/12] net: pcs: implement Firmware node support for PCS driver ---
commit 7bea9f15dae43881372af44cc139395687f3fc53
Author: Christian Marangi <[email protected]>
Date:   Thu Aug 13 08:35:08 2026 +0000

    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 .fwnode_xlate function.

> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/pcs/pcs.c
[ ... ]
> +struct fwnode_pcs_provider *
> +fwnode_pcs_add_provider(struct fwnode_handle *fwnode,
> +			struct phylink_pcs *(*fwnode_xlate)(struct fwnode_reference_args *pcsspec,
> +							    void *data),
> +			void *data)
> +{
> +	struct fwnode_pcs_provider *pp;
> +
> +	if (!fwnode)
> +		return ERR_PTR(-EINVAL);
> +
> +	pp = kzalloc_obj(*pp);
> +	if (!pp)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pp->fwnode = fwnode_handle_get(fwnode);
> +	pp->data = data;
> +	pp->fwnode_xlate = fwnode_xlate;

[Severity: High]
Should struct fwnode_pcs_provider track the module owner or device to allow
reference counting? Without it, there doesn't seem to be a way to prevent
the provider module from being unloaded while consumers hold a pointer.

> +
> +	mutex_lock(&fwnode_pcs_mutex);
[ ... ]
> +static struct phylink_pcs *
> +__fwnode_pcs_get_from_pcsspec_provider(struct fwnode_reference_args *pcsspec,
> +				       struct fwnode_pcs_provider *provider)
> +{
> +	if (provider->fwnode != pcsspec->fwnode)
> +		return ERR_PTR(-EINVAL);
> +
> +	return provider->fwnode_xlate(pcsspec, provider->data);
> +}
> +
> +static struct phylink_pcs *
> +fwnode_pcs_get_from_pcsspec(struct fwnode_reference_args *pcsspec)
> +{
> +	struct fwnode_pcs_provider *provider;
> +	struct phylink_pcs *pcs = NULL;
> +
> +	if (!pcsspec)
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&fwnode_pcs_mutex);
> +	list_for_each_entry(provider, &fwnode_pcs_providers, link) {
> +		pcs = __fwnode_pcs_get_from_pcsspec_provider(pcsspec, provider);
> +		if (!IS_ERR(pcs))
> +			break;
> +	}

[Severity: Medium]
The loop breaks only if !IS_ERR(pcs). If the fwnode matches but the
provider's fwnode_xlate function returns a legitimate error (like -EINVAL
for bad arguments), won't this loop just continue to the next provider
instead of aborting and propagating the error?

After checking all other non-matching providers, it looks like it will
unconditionally overwrite the actual error code with -ENODEV below.

> +	mutex_unlock(&fwnode_pcs_mutex);
> +
> +	return !IS_ERR_OR_NULL(pcs) ? pcs : ERR_PTR(-ENODEV);

[Severity: High]
If a provider is not yet registered, this returns -ENODEV. Should this
return -EPROBE_DEFER instead, so the consumer knows to retry when the PCS
driver probes later?

> +}
> +
> +static struct phylink_pcs *__fwnode_pcs_get(const 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);

[Severity: High]
Does this return a raw pointer to the consumer without pinning the provider
module in memory (e.g., via try_module_get)?

If the provider driver is unbound or unloaded via rmmod, could this leave
the MAC driver with a dangling pointer and lead to a use-after-free during
network operations?

> +	fwnode_handle_put(pcsspec.fwnode);
> +
> +	return pcs;
> +}
[ ... ]
> +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]
By explicitly catching and ignoring -ENODEV, doesn't this break standard
probe deferral, causing the MAC driver to initialize without its required
PCS?

Also, since the loop variable i increments but found does not when a
device is skipped, wouldn't subsequent valid PCS devices be shifted into
earlier array indices in available_pcs, corrupting the expected hardware
mapping?

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

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