Re: [PATCH v4 03/16] iio: adc: at91-sama5d2_adc: rework temp calibration layout handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `DEFINE_FREE` wrapper for `nvmem_cell_put` incorrectly handles error pointers, leading to a kernel crash when `nvmem_cell_get()` fails.
- [Medium] The introduced temperature calibration tag index and expected tag macro are never used to validate the NVMEM packet.
--

--- Patch [3]: [PATCH v4 03/16] iio: adc: at91-sama5d2_adc: rework temp calibration layout handling ---
commit 859125ad776a3a3f2a744e65f324ef58a384982e
Author: Varshini Rajendran <varshini.rajendran@microchip.com>

    iio: adc: at91-sama5d2_adc: rework temp calibration layout handling
    
    Extend support to handle different temperature calibration layouts.
    
    Add a temperature calibration data layout structure to describe indexes
    of the factors P1, P4, P6, tag, minimum length of the packet and the
    scaling factors for P1 (p1_scale) which are SoC-specific instead of the
    older non scalable id structure. This helps handle the differences in the
    same function flow and prepare the calibration data to be applied.

> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 4a4a25f3c715a..798e4d60bf697 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
[ ... ]
> @@ -2251,11 +2271,18 @@ static int at91_adc_temp_sensor_init(struct at91_adc_state *st,
>  				     struct device *dev)
>  {
>  	struct at91_adc_temp_sensor_clb *clb = &st->soc_info.temp_sensor_clb;
> +	const struct at91_adc_temp_calib_layout *layout;
>  	size_t len;
>  
>  	if (!st->soc_info.platform->temp_sensor)
>  		return 0;
>  
> +	layout = st->soc_info.platform->temp_calib_layout;
> +	if (!layout)
> +		return -ENODEV;
> +	if (!layout->p1_scale.denominator)
> +		return -EINVAL;
> +
>  	/* Get the calibration data from NVMEM. */
>  	struct nvmem_cell *temp_calib __free(nvmem_cell_put) =
>  		nvmem_cell_get(dev, "temperature_calib");

[Severity: High]
Does the DEFINE_FREE() wrapper for nvmem_cell_put() properly handle error
pointers?

If nvmem_cell_get() returns an ERR_PTR (e.g. if the NVMEM cell is missing
or defers probe), the error pointer will be truthy. If the custom
DEFINE_FREE(nvmem_cell_put) wrapper only checks "if (_T)" instead of
using IS_ERR_OR_NULL(), the truthy ERR_PTR can bypass the guard and be
passed to nvmem_cell_put(). This would unconditionally dereference the
error pointer and cause a kernel panic.

[ ... ]
> @@ -2270,20 +2297,23 @@ 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 < AT91_ADC_TS_CLB_IDX_MAX * sizeof(*buf)) {
> +	if (len < layout->min_len * sizeof(*buf)) {
>  		dev_err(dev, "Invalid calibration data!\n");
>  		return -EINVAL;
>  	}

[Severity: Medium]
Are we missing the expected packet tag validation here?

The commit introduces the AT91_TEMP_CALIB_TAG_ACST macro and the tag_idx field
specifically to validate the NVMEM cell content, but the actual validation
check seems omitted.

This might allow the driver to accept corrupted or invalid NVMEM calibration
data as long as it meets the minimum length requirement.

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