Re: [PATCH v3 phy-next 2/8] soc: fsl: guts: add a global structure to hold state

[Vladimir Oltean <vladimir.oltean@nxp.com>]

Hi Michael,

On Mon, Jul 20, 2026 at 05:01:39PM +0300, Vladimir Oltean wrote:
> On Mon, Jul 20, 2026 at 01:50:49PM +0000, sashiko-bot@kernel.org wrote:
> > [Severity: High]
> > Will jumping to err_nomem cause a NULL pointer dereference?
> > 
> > If the allocation of soc_dev_attr fails, the code now jumps to the error
> > handling path which dereferences soc_dev_attr to free its members:
> > 
> > > @@ -269,6 +271,8 @@ static int __init fsl_guts_init(void)
> > >  	kfree(soc_dev_attr->revision);
> > >  	kfree(soc_dev_attr->serial_number);
> > 
> > Since soc_dev_attr is NULL when this allocation fails, won't evaluating
> > soc_dev_attr->revision and other members result in a kernel panic?
> 
> yeah.
> 
> I tried to be nice and play with the existing "err_nomem" and "err"
> error handling labels, but the truth is that a single error cleanup path
> is just an antipattern that doesn't scale as code becomes more complex.
> 
> Instead of having a single error handling procedure which needs to
> carefully fend off from various invalid contexts from all the goto
> sites, the standard convention is to have an incremental set of labels
> which undo just the setup that was done up to the goto jump site.
> 
> I'll refactor fsl_guts_init() to use that convention for v4.

While I was doing this refactoring, I noticed that this code path:

	if (soc_data)
		soc_uid = fsl_guts_get_soc_uid(soc_data->sfp_compat,
					       soc_data->uid_offset);
	if (soc_uid) {
		soc_dev_attr->serial_number = kasprintf(GFP_KERNEL, "%016llX",
							soc_uid);
		// no kasprintf() NULL return code check here, unlike
		// the rest of the code
	}

	soc_dev = soc_device_register(soc_dev_attr);
	if (IS_ERR(soc_dev)) {
		ret = PTR_ERR(soc_dev);
		goto err;
	}

proceeds with soc_device_register() even if fsl_guts_get_soc_uid() was
able to get a soc unique id from the security fuse processor, but we
fail to print that string to the soc_dev_attr->serial_number variable.

I don't see anywhere mentioned in commit 786dde1e59d7 ("soc: fsl: guts:
add serial_number support") that this would be intentional, so it can
just as well be an omission.

Is it OK if I replace the silent failure with a loud failure of the
entire fsl_guts_init()? The code executes only if soc_uid is non-zero
anyway.

kasprintf() fails only for memory related reasons.

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