Re: [PATCH v3] net: fman: fix use-after-free on IRQF_SHARED handler after probe failure

[Simon Horman]

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
---
net: fman: fix use-after-free on IRQF_SHARED handler after probe failure

This patch fixes a use-after-free in the error path of read_dts_node(). It
manually releases the registered IRQ handlers before freeing the fman
structure, preventing the devres cleanup from leaving a registered IRQ
pointing to freed memory.

> diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
> index 013273a2de32f..ba2338da0cea1 100644
> --- a/drivers/net/ethernet/freescale/fman/fman.c
> +++ b/drivers/net/ethernet/freescale/fman/fman.c
[ ... ]
> @@ -2786,7 +2790,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
>  		if (err < 0) {
>  			dev_err(&of_dev->dev, "%s: irq %d allocation failed (error = %d)\n",
>  				__func__, fman->dts_params.err_irq, err);
> -			goto fman_free;
> +			goto free_main_irq;

[Severity: High]
This is a pre-existing issue, but since we are modifying the IRQ error
handling here, could the shared interrupt handler crash if it fires during
this setup phase?

Just before this snippet in read_dts_node(), devm_request_irq() is called
with IRQF_SHARED while fman is still partially initialized. Since fman was
allocated with kzalloc_obj(), fman->cfg and fman->fpm_regs are NULL.

If a shared interrupt fires immediately, fman_irq() is called:

drivers/net/ethernet/freescale/fman/fman.c:fman_irq() {
        ...
        if (!is_init_done(fman->cfg))
                return IRQ_NONE;

        fpm_rg = fman->fpm_regs;
        pending = ioread32be(&fpm_rg->fm_npi);
        ...
}

Since fman->cfg is NULL, is_init_done() behaves like this:

drivers/net/ethernet/freescale/fman/fman.c:is_init_done() {
        if (!cfg)
                return true;
        ...
}

Would this cause fman_irq() to bypass the early return, proceed with a NULL
fpm_rg pointer, and dereference it during the ioread32be() call?

>  		}
>  	}