Re: [PATCH RESEND v5 6/6] iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()

[Esben Haabendal <esben@geanix.com>]

<sashiko-bot@kernel.org> writes:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Low] Compilation Error in mma8452_runtime_suspend() due to undeclared identifier 'client'.
>
> Pre-existing issues:
> - [High] Missing pm_runtime_disable() in mma8452_probe() error paths leads to a guaranteed NULL pointer dereference in the autosuspend callback.
> --
>
> --- Patch [6]: [PATCH RESEND v5 6/6] iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe() ---
> commit 004c22b83c7e1b63662958bd7b1a838d3015cf83
> Author: Esben Haabendal <esben@geanix.com>
>
> iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()
>
> In commit 32a5c04d4575 ("iio: accel: mma8452: Use dev_err_probe()") the
> struct device * pointer was assigned to local variable dev, so we can just
> as well reuse that throughout the function for sligthly more readable code.
>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 060a6207e52d0..bf5cb1c4425a3 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>
> [ ... ]
>
>> @@ -1731,14 +1730,13 @@ static int mma8452_probe(struct i2c_client *client)
>>  			goto buffer_cleanup;
>>  	}
>>
>> -	ret = pm_runtime_set_active(&client->dev);
>> +	ret = pm_runtime_set_active(dev);
>>  	if (ret < 0)
>>  		goto free_irq;
>>
>> -	pm_runtime_enable(&client->dev);
>> -	pm_runtime_set_autosuspend_delay(&client->dev,
>> -					 MMA8452_AUTO_SUSPEND_DELAY_MS);
>> -	pm_runtime_use_autosuspend(&client->dev);
>> +	pm_runtime_enable(dev);
>> +	pm_runtime_set_autosuspend_delay(dev, MMA8452_AUTO_SUSPEND_DELAY_MS);
>> +	pm_runtime_use_autosuspend(dev);
>>
>>  	ret = iio_device_register(indio_dev);
>>  	if (ret < 0)
>
> [Severity: High]
> This is a pre-existing issue, but if mma8452_probe() fails here, is there a
> missing pm_runtime_disable() in the error paths?
>
> If mma8452_probe() fails after enabling PM runtime (for example, during
> iio_device_register()), it returns an error without disabling PM runtime or
> autosuspend. The driver core cleans up the device and clears its driver data
> via dev_set_drvdata(dev, NULL), and queues an idle notification.
>
> Because PM runtime is still active, this triggers the autosuspend timer.
> When the timer expires, the autosuspend callback executes:
>
> drivers/iio/accel/mma8452.c:mma8452_runtime_suspend() {
>     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>     struct mma8452_data *data = iio_priv(indio_dev);
>     ...
> }
>
> Does this lead to a reliable NULL pointer dereference when iio_priv()
> dereferences the cleared driver data?

I would think so. I will add a fix to the series, calling both
pm_runtime_disable() and pm_runtime_set_suspended().

> [Severity: Low]
> Also, looking at mma8452_runtime_suspend(), does the module fail to compile
> when CONFIG_PM is enabled due to an undeclared identifier?
>
> The code attempts to call synchronize_irq():
>
> drivers/iio/accel/mma8452.c:mma8452_runtime_suspend() {
>     ...
>     synchronize_irq(client->irq);
>     ...
> }
>
> Since client is not declared in the scope of mma8452_runtime_suspend(),
> should this access data->client->irq instead?

Ok. That is strange. I was very sure I tested this exact patch version,
but I must have made a mistake. I will fix, and make sure I test it
properly (obviously with CONFIG_PM enabled to actually test it).

/Esben