Re: [PATCH v3 2/2] iio: adc: xilinx-ams: fix out-of-bounds accesses when parsing channels
From: Sai Krishna Potthuri
Date: Fri Nov 06 2026 - 05:23:37 EST
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@xxxxxxxxxxxxxxx
Signed-off-by: Arnav Kapoor <kapoorarnav43@xxxxxxxxx>
Looks good to me.
Reviewed-by: Sai Krishna Potthuri <sai.krishna.potthuri@xxxxxxx>
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", ®);
- 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;