Re: [PATCH v2 phy-next 5/8] soc: fsl: guts: make fsl_soc_data available after fsl_guts_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The iomem mappings `soc.dcfg_ccsr` and `soc.dcfg_dcsr` are leaked on multiple error paths during initialization.
- [High] The global `soc.data`, `soc.dcfg_ccsr` and `soc.dcfg_dcsr` pointers are not cleared on error paths, leading to dangling pointers, Use-After-Free, and uninitialized spinlock access.
--

commit 9f45015d702d2f4ca41d93015c23b3d9950743a7
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

soc: fsl: guts: make fsl_soc_data available after fsl_guts_init()

In a future change, struct fsl_soc_data will be extended with methods
for performing RCW override.

Since this will be performed from a calling context outside
fsl_guts_init(), we need to keep track of the soc_data that we determine
at fsl_guts_init() time, so we can reference it later.

> diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
> index 1494b545bbb47..9f2aff07a2744 100644
> --- a/drivers/soc/fsl/guts.c
> +++ b/drivers/soc/fsl/guts.c
> @@ -243,7 +243,7 @@ static int __init fsl_guts_init(void)
>  	np = of_find_matching_node_and_match(NULL, fsl_guts_of_match, &match);
>  	if (!np)
>  		return 0;
> -	soc_data = match->data;
> +	soc.data = match->data;
>  
>  	soc.dcfg_ccsr = of_iomap(np, DCFG_CCSR);
>  	if (!soc.dcfg_ccsr) {

[Severity: High]
Does this code leak the soc.dcfg_ccsr and soc.dcfg_dcsr mappings on error paths?

Looking at fsl_guts_init(), if the soc_dev_attr allocation fails, the function
returns directly, leaking the mappings:

fsl_guts_init() {
    ...
	soc_dev_attr = kzalloc_obj(*soc_dev_attr);
	if (!soc_dev_attr)
		return -ENOMEM;
    ...
}

If soc_device_register() fails, the code jumps to the err label, which bypasses
the err_nomem block where iounmap() is called, also leaking the mappings:

fsl_guts_init() {
    ...
	soc_dev = soc_device_register(soc_dev_attr);
	if (IS_ERR(soc_dev)) {
		ret = PTR_ERR(soc_dev);
		goto err;
	}
    ...
err_nomem:
	ret = -ENOMEM;

	iounmap(soc.dcfg_ccsr);
err:
    ...
}

Could the error paths be updated to ensure iounmap() is called when these
failures occur?

[Severity: High]
Are the global soc.data, soc.dcfg_ccsr, and soc.dcfg_dcsr pointers cleared on
error paths?

In fsl_guts_init(), global state is set early:

fsl_guts_init() {
    ...
	soc.data = match->data;

	soc.dcfg_ccsr = of_iomap(np, DCFG_CCSR);
    ...
}

If an error occurs later, the memory is unmapped but the global pointers are
not cleared to NULL:

fsl_guts_init() {
    ...
err_nomem:
	ret = -ENOMEM;
	if (soc.dcfg_dcsr)
		iounmap(soc.dcfg_dcsr);
	iounmap(soc.dcfg_ccsr);
    ...
}

External callers checking if the driver is initialized via if (!soc.data) or
if (!soc.dcfg_dcsr) could bypass the checks and access the unmapped IO memory
or an uninitialized spinlock:

fsl_guts_lane_set_mode() {
    ...
	spin_lock(&soc.rcwcr_lock);

	if (soc.data->serdes_init_rcwcr)
		soc.data->serdes_init_rcwcr(serdes_idx);

	fsl_guts_rmw(override.offset, override.val << __bf_shf(override.mask),
		     override.mask);
    ...
}

Could the error paths be updated to set these pointers to NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612210859.266759-1-vladimir.oltean@nxp.com?part=5

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy