Re: [PATCH v7 03/10] iio: adc: add helpers for parsing ADC nodes

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Mar 13, 2025 at 09:18:18AM +0200, Matti Vaittinen wrote:
> There are ADC ICs which may have some of the AIN pins usable for other
> functions. These ICs may have some of the AIN pins wired so that they
> should not be used for ADC.
> 
> (Preferred?) way for marking pins which can be used as ADC inputs is to
> add corresponding channels@N nodes in the device tree as described in
> the ADC binding yaml.
> 
> Add couple of helper functions which can be used to retrieve the channel
> information from the device node.

...

> +int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
> +					  const struct iio_chan_spec *template,
> +					  int max_chan_id,
> +					  struct iio_chan_spec **cs)
> +{
> +	struct iio_chan_spec *chan_array, *chan;
> +	int num_chan = 0, ret;

Unneeded assignment.

> +	num_chan = iio_adc_device_num_channels(dev);
> +	if (num_chan < 1)
> +		return num_chan;

This is really interesting code. So, if the above returns negative error code,
we return it, if it returns 0, we return success (but 0 channels)?

Shouldn't we do *cs = NULL; at the case of 0 channels if it's a success?
(Under success I assume that returned values are okay to go with, and cs in
your case will be left uninitialised or contain something we don't control.

> +	chan_array = devm_kcalloc(dev, num_chan, sizeof(*chan_array),
> +				  GFP_KERNEL);
> +	if (!chan_array)
> +		return -ENOMEM;
> +
> +	chan = &chan_array[0];
> +
> +	device_for_each_named_child_node_scoped(dev, child, "channel") {
> +		u32 ch;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &ch);
> +		if (ret)
> +			return ret;

> +		if (max_chan_id != -1 && ch > max_chan_id)
> +			return -ERANGE;

Hmm... What if max_chan_id is equal to an error code?
Or in other words, why -1 is special and not all negative numbers?

Also note, you used unsigned type and compare it to int which,
in case of being negative will give promotion. The ch will not be
big enough in most cases (unless it's great than (INT_MAX + 1).

TL;DR: you have a potential integer overflow here.

> +		*chan = *template;
> +		chan->channel = ch;
> +		chan++;
> +	}
> +
> +	*cs = chan_array;
> +
> +	return num_chan;
> +}

-- 
With Best Regards,
Andy Shevchenko