Re: [PATCH] iio: adc: xilinx-xadc: free IRQ before cancelling the unmask worker on unbind

On 8/2/26 3:28 AM, Fan Wu wrote:
> The ZYNQ XADC interrupt handler xadc_zynq_interrupt_handler() arms the
> zynq_unmask_work delayed work via schedule_delayed_work() every time an
> alarm condition is observed, and that worker re-arms itself for as long
> as the alarm stays asserted.
> 
> In xadc_probe() the IRQ is requested with devm_request_irq() before the
> devm_add_action_or_reset() that registers xadc_cancel_delayed_work().
> Because devres release runs in LIFO order, on unbind the delayed work is
> cancelled before the IRQ is freed. The IRQ is still live at that point,
> so a pending alarm can make the handler run once more and re-arm
> zynq_unmask_work after it has been cancelled; that instance then runs
> after the xadc structure that embeds zynq_unmask_work has been freed, a
> use-after-free in xadc_zynq_unmask_worker().
> 
> Register the cancel-work devm action before requesting the IRQ so devres
> LIFO teardown frees (and synchronizes) the IRQ first, then cancels the
> delayed work. After free_irq() the handler can no longer re-arm the
> work, and the subsequent cancel_delayed_work_sync() drains any instance
> armed just before the IRQ was torn down.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: 2a9685d1a3b7 ("iio: adc: xilinx: use more devres helpers and remove remove()")
> Cc: [email protected]
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <[email protected]>
> ---
> 
>  drivers/iio/adc/xilinx-xadc-core.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -1395,13 +1395,16 @@ static int xadc_probe(struct platform_device *pdev)
>  	}
> 
>  	if (irq > 0) {
> -		ret = devm_request_irq(dev, irq, xadc->ops->interrupt_handler,
> -				       0, dev_name(dev), indio_dev);
> +		/* devm LIFO: register the cancel-work action before the IRQ,
> +		 * so unbind frees the IRQ first, then drains the work.
> +		 */
> +		ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
> +					       &xadc->zynq_unmask_work);
>  		if (ret)
>  			return ret;
> 
> -		ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
> -					       &xadc->zynq_unmask_work);
> +		ret = devm_request_irq(dev, irq, xadc->ops->interrupt_handler,
> +				       0, dev_name(dev), indio_dev);
>  		if (ret)
>  			return ret;
>  	}
> --
> 2.43.0
> 

Makes sense.

Reviewed-by: David Lechner <[email protected]>