Re: [PATCH 1/3] iio: accel: adis16201: merge adis16203 support from staging

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Stack variable address is passed to persistent state, leading to a Use-After-Free.
- [Critical] IIO device name is set to the combined driver name, breaking userspace ABI.
- [High] Missing `spi_device_id` table causes a NULL pointer dereference in the SPI core when matched via fallback.
- [High] Missing NULL check on `spi_get_device_match_data()` return value leads to NULL pointer dereference.
--

commit 4a7d80a7646bf1890c13a1b5b3c99c166b2c3898
Author: Shehryar Ahmad <shehryar.amd@gmail.com>

iio: accel: adis16201: merge adis16203 support from staging
    
This commit merges support for the ADIS16203 inclinometer into the mainline
ADIS16201 driver. It leverages of_device_id match data to handle differences
in write masks and calibration bit widths between the two chips.

> diff --git a/drivers/iio/accel/adis16201.c b/drivers/iio/accel/adis16201.c
> index 5127e58eebc7d..293655fb00531 100644
> --- a/drivers/iio/accel/adis16201.c
> +++ b/drivers/iio/accel/adis16201.c
[ ... ]
> @@ -248,56 +301,62 @@ static const struct adis_data adis16201_data = {
[ ... ]
>  static int adis16201_probe(struct spi_device *spi)
>  {
>  	struct iio_dev *indio_dev;
> -	struct adis *st;
> +	struct adis16201_state *st;
> +	struct adis_data data = adis16201_data;

[Severity: Critical]
Is it safe to allocate this struct adis_data on the stack here?

Later in this function, the address of this stack variable is passed to
adis_init(), which stores it in adis->data:

drivers/iio/imu/adis.c:adis_init() {
    ...
    adis->data = data;
    ...
}

Since the stack frame is destroyed when adis16201_probe() returns, won't any
subsequent device operation (such as reading raw values or trigger handler
execution) dereference adis->data and read unallocated stack memory?

>  	int ret;
>  
> -	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(struct adis16201_state));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
>  	st = iio_priv(indio_dev);
> +	st->info = spi_get_device_match_data(spi);
>  
>  	indio_dev->name = spi->dev.driver->name;

[Severity: Critical]
Will this change break backward compatibility for existing userspace
applications?

Previously, spi->dev.driver->name was "adis16201". Since the driver name
is updated later in this patch to "adis16201_adis16203", the exported sysfs
name attribute will change. Any userspace applications or udev rules matching
on the exact sensor name might fail.

>  	indio_dev->info = &adis16201_info;
> -
> -	indio_dev->channels = adis16201_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(adis16201_channels);
> +	indio_dev->channels = st->info->arr_chans;

[Severity: High]
Could st->info be NULL when it is dereferenced here?

If the driver is bound without match data, for example via manual sysfs
binding or legacy board files, spi_get_device_match_data() will return NULL.
This would result in a kernel oops when accessing st->info->arr_chans.

> +	indio_dev->num_channels = st->info->num_chans;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	ret = adis_init(st, indio_dev, spi, &adis16201_data);
> +	data.status_error_mask = st->info->diag_stat_mask;
> +	ret = adis_init(&st->adis, indio_dev, spi, &data);
>  	if (ret)
>  		return ret;
[ ... ]
> +static const struct of_device_id adis16201_of_match[] = {
> +	{ .compatible = "adi,adis16201", .data = &adis16201_diff },
> +	{ .compatible = "adi,adis16203", .data = &adis16203_diff },
> +	{ }
> +};
> +
>  static struct spi_driver adis16201_driver = {
>  	.driver = {
> -		.name = "adis16201",
> +		.name = "adis16201_adis16203",
> +		.of_match_table = adis16201_of_match,
>  	},
>  	.probe = adis16201_probe,
>  };

[Severity: High]
Should this struct include an id_table?

If the device is instantiated without OF/ACPI (like legacy spi_board_info or
sysfs driver_override), device_get_match_data() returns NULL. When the probe
function then calls spi_get_device_match_data(), it falls back to
spi_get_device_id().

Since the id_table is missing here, spi_get_device_id() passes a NULL pointer
to spi_match_id(), which can cause a kernel oops when it tries to access
id->name[0]. Local users with sysfs access or board initialization code could
trigger this crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831184201.34416-1-shehryar.amd@gmail.com?part=1