Re: [PATCH v3 2/2] iio: adc: xilinx-ams: fix out-of-bounds accesses when parsing channels

[Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>]

On 11/4/2026 10:15 PM, Arnav Kapoor wrote:
> ams_parse_firmware() allocates room for ARRAY_SIZE(ams_ps_channels) +
> ARRAY_SIZE(ams_pl_channels) + ARRAY_SIZE(ams_ctrl_channels) = 51
> channel specs and lets ams_init_module() fill them for the AMS node and
> each of its children, without telling it how much room is left.
> 
> For the PL-SYSMON node, ams_init_module() always adds the 10 fixed PL
> channels, numbered 20 to 29, and ams_get_ext_chan() then appends one
> channel per "channel@N" child as ams_pl_channels[N - 20]. Only channels
> 30 to 50 are external ones, but any N from 20 to 50 is accepted, so a
> child numbered 20 to 29 duplicates a fixed channel. A PL node can then
> have up to 41 channels while only 31 are budgeted for it, and together
> with the 13 PS and 7 AMS control channels this writes past the end of
> the buffer. Since the modules are filled in device tree order, the fixed
> channel blocks copied after the PL node can overflow as well.
> 
> ams_get_ext_chan() also only checks the upper bound of reg. ext_chan is
> unsigned, so a reg below 20 makes it wrap and ams_pl_channels[] is read
> far out of bounds.
> 
> Only accept the external channels 30 to 50. Also pass the remaining
> capacity down to ams_init_module() and ams_get_ext_chan() and fail with
> -ENOSPC when a module does not fit, so that a device tree repeating a
> reg value can't write past the buffer either.
> 
> Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Arnav Kapoor <kapoorarnav43@gmail.com>

Looks good to me.
Reviewed-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>

Regards
Sai krishna

> ---
>   drivers/iio/adc/xilinx-ams.c | 57 +++++++++++++++++++++++++++++-------
>   1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> index 158e6133abf5..745edcf7c735 100644
> --- a/drivers/iio/adc/xilinx-ams.c
> +++ b/drivers/iio/adc/xilinx-ams.c
> @@ -144,7 +144,14 @@
>   #define AMS_PL_CSTS_ACCESS_MASK		BIT(1)
>   
>   #define AMS_PL_MAX_FIXED_CHANNEL	10
> -#define AMS_PL_MAX_EXT_CHANNEL		20
> +/*
> + * The PL channels are numbered from 20 in the binding. The first
> + * AMS_PL_MAX_FIXED_CHANNEL of them are always present, the external ones
> + * from AMS_PL_EXT_CHANNEL_MIN to AMS_PL_EXT_CHANNEL_MAX follow them in
> + * ams_pl_channels[].
> + */
> +#define AMS_PL_EXT_CHANNEL_MIN		30
> +#define AMS_PL_EXT_CHANNEL_MAX		50
>   
>   #define AMS_INIT_POLL_TIME_US		200
>   #define AMS_INIT_TIMEOUT_US		10000
> @@ -1131,6 +1138,9 @@ static const struct iio_chan_spec ams_pl_channels[] = {
>   	AMS_PL_AUX_CHAN_VOLTAGE(15),
>   };
>   
> +static_assert(ARRAY_SIZE(ams_pl_channels) ==
> +	      AMS_PL_MAX_FIXED_CHANNEL + AMS_PL_EXT_CHANNEL_MAX - AMS_PL_EXT_CHANNEL_MIN + 1);
> +
>   static const struct iio_chan_spec ams_ctrl_channels[] = {
>   	AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCC_PSPLL, AMS_VCC_PSPLL0, "VCC_PSPLL"),
>   	AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCC_PSBATT, AMS_VCC_PSPLL3, "VCC_PSBATT"),
> @@ -1142,20 +1152,31 @@ static const struct iio_chan_spec ams_ctrl_channels[] = {
>   };
>   
>   static int ams_get_ext_chan(struct fwnode_handle *chan_node,
> -			    struct iio_chan_spec *channels, int num_channels)
> +			    struct iio_chan_spec *channels, int num_channels,
> +			    unsigned int max_channels)
>   {
>   	struct iio_chan_spec *chan;
> -	struct fwnode_handle *child;
>   	unsigned int reg, ext_chan;
>   	int ret;
>   
> -	fwnode_for_each_child_node(chan_node, child) {
> +	fwnode_for_each_child_node_scoped(chan_node, child) {
>   		ret = fwnode_property_read_u32(child, "reg", &reg);
> -		if (ret || reg > AMS_PL_MAX_EXT_CHANNEL + 30)
> +		if (ret)
> +			continue;
> +
> +		/*
> +		 * The fixed channels have been added already, anything else
> +		 * outside of the external range is not a valid channel.
> +		 */
> +		if (reg < AMS_PL_EXT_CHANNEL_MIN ||
> +		    reg > AMS_PL_EXT_CHANNEL_MAX)
>   			continue;
>   
> +		if (num_channels >= max_channels)
> +			return -ENOSPC;
> +
>   		chan = &channels[num_channels];
> -		ext_chan = reg + AMS_PL_MAX_FIXED_CHANNEL - 30;
> +		ext_chan = reg - AMS_PL_EXT_CHANNEL_MIN + AMS_PL_MAX_FIXED_CHANNEL;
>   		memcpy(chan, &ams_pl_channels[ext_chan], sizeof(*channels));
>   
>   		if (fwnode_property_read_bool(child, "xlnx,bipolar"))
> @@ -1183,7 +1204,8 @@ static void ams_iounmap_pl(void *data)
>   
>   static int ams_init_module(struct iio_dev *indio_dev,
>   			   struct fwnode_handle *fwnode,
> -			   struct iio_chan_spec *channels)
> +			   struct iio_chan_spec *channels,
> +			   unsigned int max_channels)
>   {
>   	struct device *dev = indio_dev->dev.parent;
>   	struct ams *ams = iio_priv(indio_dev);
> @@ -1191,6 +1213,9 @@ static int ams_init_module(struct iio_dev *indio_dev,
>   	int ret;
>   
>   	if (fwnode_device_is_compatible(fwnode, "xlnx,zynqmp-ams-ps")) {
> +		if (max_channels < ARRAY_SIZE(ams_ps_channels))
> +			return -ENOSPC;
> +
>   		ams->ps_base = fwnode_iomap(fwnode, 0);
>   		if (!ams->ps_base)
>   			return -ENXIO;
> @@ -1202,6 +1227,9 @@ static int ams_init_module(struct iio_dev *indio_dev,
>   		memcpy(channels, ams_ps_channels, sizeof(ams_ps_channels));
>   		num_channels = ARRAY_SIZE(ams_ps_channels);
>   	} else if (fwnode_device_is_compatible(fwnode, "xlnx,zynqmp-ams-pl")) {
> +		if (max_channels < AMS_PL_MAX_FIXED_CHANNEL)
> +			return -ENOSPC;
> +
>   		ams->pl_base = fwnode_iomap(fwnode, 0);
>   		if (!ams->pl_base)
>   			return -ENXIO;
> @@ -1213,9 +1241,14 @@ static int ams_init_module(struct iio_dev *indio_dev,
>   		/* Copy only first 10 fix channels */
>   		memcpy(channels, ams_pl_channels, AMS_PL_MAX_FIXED_CHANNEL * sizeof(*channels));
>   		num_channels += AMS_PL_MAX_FIXED_CHANNEL;
> -		num_channels = ams_get_ext_chan(fwnode, channels,
> -						num_channels);
> +		ret = ams_get_ext_chan(fwnode, channels, num_channels, max_channels);
> +		if (ret < 0)
> +			return ret;
> +		num_channels = ret;
>   	} else if (fwnode_device_is_compatible(fwnode, "xlnx,zynqmp-ams")) {
> +		if (max_channels < ARRAY_SIZE(ams_ctrl_channels))
> +			return -ENOSPC;
> +
>   		/* add AMS channels to iio device channels */
>   		memcpy(channels, ams_ctrl_channels, sizeof(ams_ctrl_channels));
>   		num_channels += ARRAY_SIZE(ams_ctrl_channels);
> @@ -1245,7 +1278,7 @@ static int ams_parse_firmware(struct iio_dev *indio_dev)
>   		return -ENOMEM;
>   
>   	if (fwnode_device_is_available(fwnode)) {
> -		ret = ams_init_module(indio_dev, fwnode, ams_channels);
> +		ret = ams_init_module(indio_dev, fwnode, ams_channels, ams_size);
>   		if (ret < 0)
>   			return ret;
>   
> @@ -1253,7 +1286,9 @@ static int ams_parse_firmware(struct iio_dev *indio_dev)
>   	}
>   
>   	device_for_each_child_node_scoped(dev, child) {
> -		ret = ams_init_module(indio_dev, child, ams_channels + num_channels);
> +		ret = ams_init_module(indio_dev, child,
> +				      ams_channels + num_channels,
> +				      ams_size - num_channels);
>   		if (ret < 0)
>   			return ret;
>