Re: [PATCH] iio: imu: st_lsm6dsx: Fix reading array out of bounds

[Alexandre Bard <alexandre.bard@netmodule.com>]

> On Thu, Apr 09, 2020 at 01:50:24PM +0200, Alexandre Bard wrote:
>> Le 09.04.20 à 13:01, Stephan Gerhold a écrit :
>>> On Thu, Apr 09, 2020 at 10:58:18AM +0200, Alexandre Bard wrote:
>>>> Former code was iterating through all possible IDs whereas only a few
>>>> per settings array are really available. Leading to several out of
>>>> bounds readings.
>>>>
>>>> Line is now longer than 80 characters. But since it is a classic for
>>>> loop I think it is better to keep it like this than splitting it.
>>>>
>>>> Signed-off-by: Alexandre Bard <alexandre.bard@netmodule.com>
>>>> ---
>>>>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>>>> index 84d219ae6aee..be8882ff30eb 100644
>>>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>>>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>>>> @@ -1350,7 +1350,7 @@ static int st_lsm6dsx_check_whoami(struct st_lsm6dsx_hw *hw, int id,
>>>>  	int err, i, j, data;
>>>>  
>>>>  	for (i = 0; i < ARRAY_SIZE(st_lsm6dsx_sensor_settings); i++) {
>>>> -		for (j = 0; j < ST_LSM6DSX_MAX_ID; j++) {
>>>> +		for (j = 0; j < ARRAY_SIZE(st_lsm6dsx_sensor_settings[i].id); j++) {
>>> id in st_lsm6dsx_settings is declared as:
>>>
>>> 	struct {
>>> 		enum st_lsm6dsx_hw_id hw_id;
>>> 		const char *name;
>>> 	} id[ST_LSM6DSX_MAX_ID];
>>>
>>> so it's always ST_LSM6DSX_MAX_ID long
>>> (additional entries are just zero-initialized).
>>>
>>> Isn't ARRAY_SIZE(st_lsm6dsx_sensor_settings[i].id) == ST_LSM6DSX_MAX_ID
>>> in this case?
>> Yes, you are right, I missed that. But there is still a problem :
>> parsing 0-initialized fields can lead to a false positive when looking for the
>> value ST_LSM6DS3_ID which is the first element of an enum. So either the enum
>> must be patched to start at 1 or the length of valid ids in a settings must be
>> retrieved somehow.
>>
>> Or is there another way ? Or am I wrong ?
> ST_LSM6DS3_ID was indeed broken, which is why I added a .name != NULL
> check in commit fb4fbc8904e7 ("iio: imu: st_lsm6dsx: Fix selection of ST_LSM6DS3_ID").
>
> .name is only set for properly initialized IDs, so this ensures that we
> do not match any zero-initialized entries. :)

Right, I actually fell on this problem in an older version where .name did not
exist and I did not understand that it was added for this purpose when I checked
out the master branch.

Looks alright then.
Thanks for the feedback.

Best regards,
Alexandre Bard
>
>>>>  			if (st_lsm6dsx_sensor_settings[i].id[j].name &&
>>>>  			    id == st_lsm6dsx_sensor_settings[i].id[j].hw_id)
>>>>  				break;
>>>> -- 
>>>> 2.20.1
>>>>