Re: [PATCH v5 05/17] iio: adc: at91-sama5d2_adc: add condition to validate calibration data

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cancel_work_sync() in at91_adc_probe() error paths causes a Use-After-Free if the probe fails after an interrupt schedules the work.
--

--- Patch [5]: [PATCH v5 05/17] iio: adc: at91-sama5d2_adc: add condition to validate calibration data ---
commit 380f9b2fd1800d246c63262001982d55d2628987
Author: Varshini Rajendran <[email protected]>

iio: adc: at91-sama5d2_adc: add condition to validate calibration data

This commit adds an additional condition to validate the calibration data
read from the nvmem cell. It checks the tag of the packet to ensure the
data is valid before proceeding.

> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 82db8f2f4db03..5b21367cf6b40 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -2297,7 +2297,8 @@ static int at91_adc_temp_sensor_init(struct at91_adc_state *st,
>  		return dev_err_probe(dev, PTR_ERR(buf),
>  				     "Failed to read calibration data!\n");
>  
> -	if (len < layout->min_len * sizeof(*buf)) {
> +	if (len < layout->min_len * sizeof(*buf) ||
> +	    buf[layout->tag_idx] != AT91_TEMP_CALIB_TAG_ACST) {

[Severity: High]
This isn't a bug introduced by this patch, but missing cancel_work_sync() in
the error paths of at91_adc_probe() can cause a use-after-free.

If the hardware interrupt is left asserted by a prior boot stage,
devm_request_irq() unmasks it before the device is reset in
at91_adc_hw_init().

The interrupt handler at91_adc_interrupt() can then run, hit the touch sense
condition, and call schedule_work().

If a subsequent initialization step in probe() fails, the probe aborts:

drivers/iio/adc/at91-sama5d2_adc.c:at91_adc_probe() {
    ...
	ret = clk_prepare_enable(st->per_clk);
	if (ret)
		goto vref_disable;
    ...
vref_disable:
	regulator_disable(st->vref);
reg_disable:
	regulator_disable(st->reg);
	return ret;
}

This implicitly frees devres allocated memory without canceling the pending
work, allowing the work to execute after st is freed.

Should there be a call to cancel_work_sync() to clear the pending work in the
error paths here?

>  		dev_err(dev, "Invalid calibration data!\n");
>  		return -EINVAL;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5