Re: [PATCH] iio: adc: xilinx-ams: fix OOB read in ams_get_ext_chan()

[Jonathan Cameron]

On 6 Sep 2026 16:35:18 +0530
"Manush Prajwal" <manushprajwal555@xxxxxxxxx> wrote:

> The PL external-channel "reg" property is only checked against its
> upper bound (AMS_PL_MAX_EXT_CHANNEL + 30 == 50), matching the
> 'maximum: 50' constraint in the devicetree binding
> (Documentation/devicetree/bindings/iio/adc/xlnx,zynqmp-ams.yaml), but
> the binding also documents 'minimum: 20' which the driver never
> enforces at runtime.
> 
> ext_chan is computed as 'reg + AMS_PL_MAX_FIXED_CHANNEL - 30' in
> unsigned arithmetic. For any reg < 20 (e.g. a hand-written or
> malformed devicetree overlay with reg = <0>), this underflows to a
> huge unsigned value, and the following
> 
> 	memcpy(chan, &ams_pl_channels[ext_chan], sizeof(*channels));
> 
> reads far outside the 31-entry ams_pl_channels[] array.
> 
> Reject any reg value that would produce an out-of-range ext_chan
> before it is used to index ams_pl_channels[], instead of relying only
> on the upper-bound check.
> 
> Signed-off-by: Manush Prajwal <manushprajwal555@xxxxxxxxx>

Explanation seems valid. I'll wait for the AMD folk to have
time to take a look though before considering picking this up.

One comment inline.
> ---
>  drivers/iio/adc/xilinx-ams.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> index 158e6133a..cd778d053 100644
> --- a/drivers/iio/adc/xilinx-ams.c
> +++ b/drivers/iio/adc/xilinx-ams.c
> @@ -1154,8 +1154,11 @@ static int ams_get_ext_chan(struct fwnode_handle *chan_node,
>  		if (ret || reg > AMS_PL_MAX_EXT_CHANNEL + 30)
>  			continue;
>  
> -		chan = &channels[num_channels];
>  		ext_chan = reg + AMS_PL_MAX_FIXED_CHANNEL - 30;
> +		if (ext_chan >= ARRAY_SIZE(ams_pl_channels))
> +			continue;
I haven't looked closely but is it ever fine to go off the top of this?
If not, error out and fail probe.
I'd also prefer to have more direct handling of the wrap around case
rather than relying on it being a big value and so failing this.

Thanks,

Jonathan

> +
> +		chan = &channels[num_channels];
>  		memcpy(chan, &ams_pl_channels[ext_chan], sizeof(*channels));
>  
>  		if (fwnode_property_read_bool(child, "xlnx,bipolar"))