Re: [PATCH v1 11/13] dt-bindings: iio: adc: adi,ad4134: Add high data throughput example

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The newly added device tree example includes 'mux-states' and 'mux-state-names' properties, but these are not defined in the schema, causing schema validation to fail.

Pre-existing issues:
- [High] Out-of-bounds array write in `ad4134_trigger_handler` due to incorrect loop bounds over `masklength`.
- [High] Timestamp push fails with `-ENOSPC` because the buffer size passed to `iio_push_to_buffers_with_ts` is undersized.
--

commit 9db0b7d2cda5447620683544f83d4587b18252b7
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

dt-bindings: iio: adc: adi,ad4134: Add high data throughput example

Add device tree example for AD4134 operation in 4-wire SPI mode with 4
lines to transfer data samples to the host controller.

> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml
> index 34043f89730b2..6f0b4346426a0 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml

[ ... ]

> +            clocks = <&sys_clk>;
> +            clock-names = "clkin";
> +
> +            avdd5-supply = <&avdd5>;
> +            dvdd5-supply = <&dvdd5>;
> +            iovdd-supply = <&iovdd>;
> +            refin-supply = <&refin>;
> +            avdd1v8-supply = <&avdd1v8>;
> +            dvdd1v8-supply = <&dvdd1v8>;
> +            clkvdd-supply = <&clkvdd>;
> +
> +            mux-states = <&ad4134_sdo_dout0_mux 0>, <&ad4134_sdo_dout0_mux 1>;
> +            mux-state-names = "data_read", "reg_access";

[Severity: Low]
Are the mux-states and mux-state-names properties fully documented in the
schema definition?

The newly added device tree example includes these properties, but they do
not appear to be defined in the schema properties section. Since the schema
uses unevaluatedProperties: false, the example will fail schema validation
during a dt_binding_check.

> +
> +            adi,spi-mode = "4-wire";
> +        };
> +    };
>  ...

While reviewing this series, I also noticed a couple of pre-existing issues
in the driver implementation:

drivers/iio/adc/ad4134.c:ad4134_trigger_handler()
{
[ ... ]
	for (unsigned int ch = 0; ch < iio_get_masklength(indio_dev); ch++) {
		ret = spi_write_then_read(st->spi, NULL, 0, &st->scan[ch],
					  BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS));
[ ... ]

[Severity: High]
This is a pre-existing issue, but does this loop overflow the st->scan array
when the soft timestamp channel is enabled?

The loop unconditionally iterates up to iio_get_masklength(indio_dev).
Because the soft timestamp channel is registered with scan_index = 4, the
mask length evaluates to 5 whenever the timestamp is enabled.

Since st->scan is an array of size 4, when ch evaluates to 4,
spi_write_then_read() will write 3 bytes out-of-bounds to &st->scan[4],
corrupting adjacent memory.

[ ... ]
	iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
				    pf->timestamp);
[ ... ]

[Severity: High]
This is a pre-existing issue, but will the statically sized length passed
here cause iio_push_to_buffers_with_ts() to fail with -ENOSPC?

The iio_push_to_buffers_with_ts() function requires the buffer size argument
to be at least indio_dev->scan_bytes to accommodate all active channels plus
the 8-byte timestamp.

The driver statically passes sizeof(st->scan), which is 16 bytes. When 3 or 4
channels plus the timestamp are active, the required size expands to 24 bytes
(16 bytes of data + 8 bytes timestamp). This will cause the push to abort,
resulting in lost samples.
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788368334.git.marcelo.schmitt@analog.com?part=11