Re: [PATCH 3/8] iio: adc: stm32-adc: add support for stm32mp23 and stm32mp25

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing bounds check on the `st,adc-trigger-sel` device tree property allows an out-of-bounds array read when indexing `adc->cfg->trigs`.
- [Medium] Missing ADC stop (`ADSTP`) on calibration timeout error path leaves ADC in a locked state.

Pre-existing issues:
- [High] Configuring an unsupported internal channel does not abort setup, leading to errant read-modify-write operations on register offset 0 (ISR), which silently clears unrelated pending interrupts.
--

commit 07a141434d979402aadc3f3eaff7d932ee702232
Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

iio: adc: stm32-adc: add support for stm32mp23 and stm32mp25

This commit adds support for the STM32MP23 and STM32MP25 SoCs to the
stm32-adc driver. It introduces support for diversity in IRQs, clocks,
internal voltage channels, and trigger lists by utilizing a dedicated
compatible string and configuration data.

> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
[ ... ]
> @@ -1503,6 +1503,163 @@ static void stm32h7_adc_unprepare(struct iio_dev *indio_dev)
[ ... ]
> +static int stm32mp25_adc_prepare(struct iio_dev *indio_dev)
> +{
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = stm32h7_adc_exit_pwr_down(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	/* Must enable the ADC before running software-assisted calibration */
> +	ret = stm32h7_adc_enable(indio_dev);
> +	if (ret)
> +		goto pwr_dwn;
> +
> +	/* Always run offset calibration */
> +	ret = stm32mp25_adc_calib(indio_dev);
> +	if (ret)
> +		goto adc_dis;
> +
> +	stm32_adc_int_ch_enable(indio_dev);
> +
> +	stm32_adc_writel(adc, adc->cfg->regs->difsel.reg, adc->difsel);
> +
> +	if (adc->cfg->has_presel)
> +		stm32_adc_writel(adc, STM32H7_ADC_PCSEL, adc->pcsel);
> +
> +	return 0;
> +
> +adc_dis:
> +	stm32h7_adc_disable(indio_dev);

[Severity: Medium]
If calibration times out, does this error path leave the ADC in a locked
state?

If stm32mp25_adc_calib() times out waiting for ADSTART to clear, it returns
an error that jumps to adc_dis. The error path directly calls
stm32h7_adc_disable(), which tries to disable the ADC by setting ADDIS.

However, the hardware specification generally prohibits setting ADDIS while
ADSTART=1. Should the driver first abort the conversion using an
stm32h7_adc_stop_conv() call to set ADSTP and wait for ADSTART to clear
before disabling the hardware?

[ ... ]
> @@ -2641,6 +2641,11 @@ static int stm32_adc_populate_int_ch(struct iio_dev *indio_dev, const char *ch_n
>  					dev_warn(&indio_dev->dev,
>  						 "%s channel not available\n", ch_name);
>  				break;
> +			case STM32_ADC_INT_CH_VDDGPU:
> +				if (!adc->cfg->regs->or_vddgpu.reg)
> +					dev_warn(&indio_dev->dev,
> +						 "%s channel not available\n", ch_name);
> +				break;
>  			}
>  
>  			if (stm32_adc_ic[i].idx != STM32_ADC_INT_CH_VREFINT) {
>  				adc->int_ch[i] = chan;
>  				break;
>  			}

[Severity: High]
This is a pre-existing issue, but does this warning branch fail to abort
channel registration?

If an unsupported internal channel like vddgpu is requested, the driver
prints a warning but falls through and assigns it a valid int_ch below.

This causes a problem later during setup:

stm32_adc_int_ch_enable() {
    ...
    case STM32_ADC_INT_CH_VDDGPU:
        stm32_adc_set_bits(adc, adc->cfg->regs->or_vddgpu.reg,
                           adc->cfg->regs->or_vddgpu.mask);
    ...
}

Because the register offset is uninitialized (0), this performs a
read-modify-write on offset 0, which corresponds to the ISR register. Since
the ISR has read-clear-write-1 semantics, this will silently clear all
currently pending unrelated interrupts.

[ ... ]
> @@ -2914,6 +2914,18 @@ static int stm32_adc_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (device_property_present(&pdev->dev, "st,adc-trigger-sel")) {
> +		ret = device_property_read_u32(dev, "st,adc-trigger-sel", &trig_id);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	adc->trigs = adc->cfg->trigs[trig_id];

[Severity: High]
Could an invalid st,adc-trigger-sel value lead to an out-of-bounds read
here?

The st,adc-trigger-sel property is read directly into trig_id and
immediately used to index the adc->cfg->trigs array, which has a fixed
size of 2. 

If a malformed Device Tree provides a larger value, this skips bounds
validation and might cause a memory safety issue by dereferencing
invalid pointers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-adc-stm32mp25-v1-v1-0-cef26eecdef2@foss.st.com?part=3