Re: [PATCH 01/10] EDAC/altera: Fix NULL of_node dereference altr_edac_device_probe()

[Borislav Petkov]

On Mon, Jul 27, 2026 at 08:24:07AM -0500, Dinh Nguyen wrote:
> In altr_edac_device_probe() dereferenced pdev->dev.of_node and
> of_match_node() before validating it,

How did that AI let you fumble that sentence? :-P

> which could cause a NULL pointer
> dereference when the device tree node is missing. Check the result for
> a NULL and bail out with -ENODEV before using it.
> 
> Assisted-by: Cursor:claude-4.8-opus
> Fixes: c3eea1942a16 ("EDAC, altera: Add Altera L2 cache and OCRAM support")
> Closes: https://sashiko.dev/#/patchset/20260719211238.589402-1-rosenp%40gmail.com

So the AI review for this one here is this one, I think:

"This isn't a bug introduced by this patch, but can np be NULL here if the
driver is bound via sysfs?
If a privileged user unbinds and rebinds the driver via sysfs to a platform
device that lacks an of_node, pdev->dev.of_node will be NULL, which would
cause an unconditional dereference when accessing np->name."

Can that happen?

Have you been able to trigger it?

If so, then the mechanism how this happens should be in the commit message,
explaining why this fix is needed and why it is needed now, all of a sudden.

AI is helpful in some cases but I would like us to sanity-check everything it
hallucinates pls.

> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> ---
>  drivers/edac/altera_edac.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index 68846f583eeef..d6ca68d74f78a 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -710,9 +710,18 @@ static int altr_edac_device_probe(struct platform_device *pdev)
>  	struct resource *r;
>  	int res = 0;
>  	struct device_node *np = pdev->dev.of_node;
> -	char *ecc_name = (char *)np->name;
> +	const struct of_device_id *edac_dev_match;
> +	char *ecc_name;
>  	static int dev_instance;

I know this is not something you should do now but let's pay attention to
this:

The preferred ordering of variable declarations at the beginning of a function
is reverse fir tree order::

	struct long_struct_name *descriptive_name;
	unsigned long foo, bar;
	unsigned int tmp;
	int ret;

The above is faster to parse than the reverse ordering::

	int ret;
	unsigned int tmp;
	unsigned long foo, bar;
	struct long_struct_name *descriptive_name;

And even more so than random ordering::

	unsigned long foo, bar;
	int ret;
	struct long_struct_name *descriptive_name;
	unsigned int tmp;
  
> +	if (!np) {
> +		edac_printk(KERN_ERR, EDAC_DEVICE,
> +			    "Unable to get device tree node\n");
> +		return -ENODEV;
> +	}

Pls write it like this:

	np = pdev->dev.of_node;
	if (!np)
		...

> +
> +	ecc_name = (char *)np->name;
> +
>  	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
>  		edac_printk(KERN_ERR, EDAC_DEVICE,
>  			    "Unable to open devm\n");

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette