Re: [PATCH v4 2/6] iio: accel: mma8452: Optimize struct mm8452_data member orders

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

Pre-existing issues:
- [High] AB-BA deadlock between the driver's data->lock and the PM runtime core synchronization.
- [High] Short I2C reads leak uninitialized kernel stack memory to userspace.
--

--- Patch [2]: [PATCH v4 2/6] iio: accel: mma8452: Optimize struct mm8452_data member orders ---
commit 6be65aa63f260fa43ab647c9d5c94e2ecf3e69dd
Author: Esben Haabendal <[email protected]>

iio: accel: mma8452: Optimize struct mm8452_data member orders

Reorder struct mma8452_data members to avoid holes.

> iio: accel: mma8452: Optimize struct mm8452_data member orders

This isn't a bug, but there's a typo (mm8452) in the subject line.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7d683686dd9d6..f645a5c6fd1c2 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -106,10 +106,7 @@ struct mma8452_data {
>  	struct i2c_client *client;
>  	struct mutex lock;

[Severity: High]
This is a pre-existing issue, but is there an AB-BA deadlock between the
driver's lock and the PM runtime core synchronization?

If a sysfs read calls mma8452_read_raw(), which acquires the lock:

mma8452_read_raw()
    mutex_lock(&data->lock);
    ret = mma8452_read(data, buffer);

And then mma8452_read() attempts to acquire a PM runtime reference by calling
pm_runtime_resume_and_get(), it blocks waiting for a suspend operation to
complete if the device is auto-suspending.

Concurrently, if the PM autosuspend timer expires, the PM core executes the
driver's suspend callback:

mma8452_runtime_suspend()
    mutex_lock(&data->lock);
    ret = mma8452_standby(data);
    mutex_unlock(&data->lock);

Could this result in a permanent deadlock if the sysfs read thread holds the
lock while waiting for the PM core, which is in turn waiting for the lock in
mma8452_runtime_suspend()?

>  	struct iio_mount_matrix orientation;
> -	u8 ctrl_reg1;
> -	u8 data_cfg;
>  	const struct mma_chip_info *chip_info;
> -	int sleep_val;
>  	struct regulator *vdd_reg;
>  	struct regulator *vddio_reg;
>  
> @@ -118,6 +115,10 @@ struct mma8452_data {
>  		__be16 channels[3];
>  		aligned_s64 ts;
>  	} buffer;

[Severity: High]
This isn't a bug introduced by this patch, but do short I2C reads leak
uninitialized kernel stack memory to userspace here?

In mma8452_read_raw(), an uninitialized buffer is allocated on the stack and
passed to mma8452_read():

mma8452_read_raw()
    __be16 buffer[3];
    ...
    ret = mma8452_read(data, buffer);

In mma8452_read(), an I2C read is performed:

mma8452_read()
    ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
                                        3 * sizeof(__be16), (u8 *)buf);
    if (ret < 0)
        return ret;

    ret = mma8452_set_runtime_pm_state(data->client, false);

    return ret;

If the I2C device returns fewer bytes than expected (a short read),
i2c_smbus_read_i2c_block_data() returns a positive integer less than 6.

The code only checks for ret < 0, and then overwrites the short read length by
returning the result of mma8452_set_runtime_pm_state(), which is 0 on success.

Does this hide the short read from the caller, causing mma8452_read_raw() to
extract a value from uninitialized stack memory and return it to userspace via
the sysfs interface?

> +
> +	int sleep_val;
> +	u8 ctrl_reg1;
> +	u8 data_cfg;
>  };
>  
>   /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2