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

[赵金明]

Hi,

The analysis is logically correct. Since fman is zero-initialized by
kzalloc_obj(), both fman->cfg and fman->fpm_regs are NULL when
devm_request_irq() registers the shared IRQ handler. The guard in
fman_irq():

    if (!is_init_done(fman->cfg))
        return IRQ_NONE;

does not protect against this case because is_init_done(NULL) returns
true, so the handler would proceed to dereference the NULL
fpm_regs pointer via ioread32be().

However, this is a pre-existing issue unrelated to the UAF fix in this
patch. The window is very short -- between devm_request_irq() and the
completion of fman_init() -- and would require another device on the
same shared IRQ line to fire an interrupt during that interval.

If this should be addressed, I will send a separate patch for it.
The current patch is focused solely on the post-IRQ-registration UAF
on error paths.

Please let me know if you would like me to handle this separately.

Best regards,
ZhaoJinming



>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?



>



>>? 		}



>>? 	}



>