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

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2871 bytes --]

Hi Vladimir,

On Mon Jul 20, 2026 at 4:57 PM CEST, Vladimir Oltean wrote:
> 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

It's a long time ago, but it seems to be designed be optional. In
that case a NULL pointer check was omitted, because nothing bad can
happen anyway. Either you'll free the string if != NULL or you don't
free anything.

> 	}
>
> 	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.

I don't have a strong opinion. If you ever get that error, something
is seriously bad anyway.

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]