Re: [PATCH v4 04/16] iio: adc: at91-sama5d2_adc: add condition to validate calibration data

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Invalid pointer dereference and kernel panic in the DEFINE_FREE(nvmem_cell_put) macro error handling path.

Pre-existing issues:
- [High] Race condition during probe: IRQ is requested before the peripheral clock is enabled and hardware interrupts are masked.
--

--- Patch [4]: [PATCH v4 04/16] iio: adc: at91-sama5d2_adc: add condition to validate calibration data ---
commit 9e92e8909ac4316dc3487b92319cfbc6db940b7b
Author: Varshini Rajendran <varshini.rajendran@microchip.com>
Subject: iio: adc: at91-sama5d2_adc: add condition to validate calibration data

This commit adds an additional check to validate the temperature calibration
data read from NVMEM. It verifies that the tag matches the expected
AT91_TEMP_CALIB_TAG_ACST value before proceeding.

> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 798e4d60bf697..15dc88f4961b2 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) ||

[Severity: Critical]
Does the error handling path in at91_adc_temp_sensor_init() safely handle
nvmem_cell_get() failures?

Looking at the initialization of temp_calib earlier in this function:

drivers/iio/adc/at91-sama5d2_adc.c:at91_adc_temp_sensor_init() {
        ...
        struct nvmem_cell *temp_calib __free(nvmem_cell_put) =
                nvmem_cell_get(dev, "temperature_calib");
        if (IS_ERR(temp_calib)) {
                ...
                return PTR_ERR(temp_calib);
        }
        ...
}

When nvmem_cell_get() fails (e.g., returning -ENOENT), it returns an ERR_PTR
and we return early. This triggers the __free(nvmem_cell_put) cleanup handler.

The cleanup handler is defined as:
DEFINE_FREE(nvmem_cell_put, struct nvmem_cell *, if (_T) nvmem_cell_put(_T))

Since the ERR_PTR is not NULL, the condition if (_T) evaluates to true,
passing the ERR_PTR directly to nvmem_cell_put(). Inside nvmem_cell_put(),
the pointer is dereferenced unconditionally, which can lead to a panic.

Should the cleanup macro be updated to check if (!IS_ERR_OR_NULL(_T)) instead?

> +	    buf[layout->tag_idx] != AT91_TEMP_CALIB_TAG_ACST) {
>  		dev_err(dev, "Invalid calibration data!\n");
>  		return -EINVAL;
>  	}

[Severity: High]
This is a pre-existing issue, but is there a race condition during probe
between requesting the IRQ and enabling the peripheral clock?

In at91_adc_probe(), devm_request_irq() is called before the peripheral clock
is enabled with clk_prepare_enable() and before at91_adc_hw_init() masks
hardware interrupts.

If an interrupt is left pending by the bootloader, the handler
at91_adc_interrupt() will execute immediately:

drivers/iio/adc/at91-sama5d2_adc.c:at91_adc_interrupt() {
        ...
        at91_adc_irq_status(st, &status, &eoc);
        ...
}

This handler reads a hardware register while the peripheral clock is still
disabled, which can cause a hardware abort. Additionally, since the interrupts
haven't been masked yet, it might bypass the IRQ_NONE check and fall through
to the software conversion branch, dereferencing the uninitialized st->chan
pointer.

Can the IRQ request be moved after the clock is enabled and the hardware is
fully initialized?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727122633.117435-1-varshini.rajendran@microchip.com?part=4