Re: [PATCH net v2 1/2] net: fman: move IRQ registration after init to prevent NULL deref and UAF

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev

Simon says:

I think we can treat the pre-existing issue flagged below in the context of
possible follow-up. But I'm wondering if you could take a look over
the first issue.

---
net: fman: move IRQ registration after init to prevent NULL deref and UAF

This commit moves IRQ registration in the fman driver from read_dts_node() to
fman_probe() after fman_init() has completed. The intention is to avoid a NULL
dereference or use-after-free caused by shared interrupts firing before the
driver state is fully initialized.

Link: https://lore.kernel.org/netdev/20260626162323.GE1310988@horms.kernel.org/

> diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
> index 299bab043175b..6947f3bc7c87e 100644
> --- a/drivers/net/ethernet/freescale/fman/fman.c
> +++ b/drivers/net/ethernet/freescale/fman/fman.c

[ ... ]

> @@ -2848,6 +2829,35 @@ static int fman_probe(struct platform_device *of_dev)
>  		return -EINVAL;
>  	}
>  
> +	/* Register IRQ handlers only after initialization is complete.
> +	 * This prevents two issues:
> +	 * 1) Pre-init NULL dereference: is_init_done(NULL) returns true,
> +	 *    so a shared-IRQ spurious firing before fpm_regs is set would
> +	 *    dereference NULL.
> +	 * 2) Use-after-free on probe failure: fman was kzalloc'd (not devm),
> +	 *    so on error paths kfree(fman) ran before devm_free_irq, leaving
> +	 *    a window where the handler could fire with a freed dev_id.
> +	 * By registering here, both problems are eliminated.
> +	 */
> +	err = devm_request_irq(dev, fman->dts_params.irq, fman_irq,
> +			       IRQF_SHARED, "fman", fman);

Simon says:

It seems to me that one possible way to address the raced flagged
below would be to register the handler earlier, but have a flag
so that that the handler is effectively an noop until initialisation
is complete.

[Severity: High]
Can this race condition lead to an unhandled interrupt storm and disable the
shared IRQ line?

By moving the IRQ registration here, the handler is registered after
fman_init() has completed. The fman_init() function configures the hardware
and enables its interrupts (e.g. via fpm_init() and enable()). Because the
interrupt is shared (IRQF_SHARED), the IRQ line might already be unmasked by
another device.

If the FMan hardware asserts an interrupt in the window between fman_init()
completing and this handler being registered, the shared IRQ will fire. The
other device's handler will return IRQ_NONE, and since the FMan handler is
absent, no one will clear the FMan interrupt.

This interrupt storm would cause the core IRQ subsystem to permanently
disable the shared IRQ line.

[Severity: High]
This is a pre-existing issue, but does the driver leak dynamically allocated
resources when unbound?

The FMan driver allocates several non-devm-managed resources (e.g., fman
via kzalloc_obj, fman->state, fman->cfg, and MURAM allocations). However,
the platform_driver structure completely lacks a .remove callback.

If the device is unbound via sysfs or the module is unloaded, the core
driver subsystem tears down devm resources like IRQs, but the dynamically
allocated FMan structures and MURAM pools appear to be permanently leaked.

[Severity: High]
This is a pre-existing issue, but can child devices (MACs) probe before the
FMan parent initialization is complete, leading to permanent probe failures?

In fman_probe(), read_dts_node() is called first, which calls
of_platform_populate() to create child MAC devices before the FMan parent
has finished initializing:

fman_probe() {
    fman = read_dts_node(of_dev); /* Calls of_platform_populate() */
    ...
    err = fman_config(fman);
    ...
    if (fman_init(fman) != 0) {
    ...
    dev_set_drvdata(dev, fman);
}

If the MAC driver is already registered when of_platform_populate() is
called, the MAC devices will probe synchronously. Their probe routine
calls fman_bind(), which reads the parent's driver data via
dev_get_drvdata().

Since dev_set_drvdata() is not called until the very end of fman_probe(),
fman_bind() would return NULL, causing the MAC probe to fail with -ENODEV.

> +	if (err < 0) {
> +		dev_err(dev, "%s: irq %d allocation failed (error = %d)\n",
> +			__func__, fman->dts_params.irq, err);
> +		return err;
> +	}