linux-kernel - Re: [PATCH] iio: imu: st_lsm6dsx: Fix an array overflow in st_lsm6dsx_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 10 Oct 2021 12:03:14 +0200
From:   Lars-Peter Clausen <lars@...afoo.de>
To:     Teng Qi <starmiku1207184332@...il.com>,
        lorenzo.bianconi83@...il.com, jic23@...nel.org
Cc:     linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        islituo@...il.com, baijiaju1990@...il.com,
        TOTE Robot <oslab@...nghua.edu.cn>
Subject: Re: [PATCH] iio: imu: st_lsm6dsx: Fix an array overflow in
 st_lsm6dsx_set_odr()

On 10/10/21 11:12 AM, Teng Qi wrote:
> The length of hw->settings->odr_table is 2 and ref_sensor->id is an enum
> variable whose value is between 0 and 5.
> However, the value ST_LSM6DSX_ID_MAX (i.e. 5) is not catched properly in
>   switch (sensor->id) {
>
> If ref_sensor->id is ST_LSM6DSX_ID_MAX, an array overflow will ocurrs in
> function st_lsm6dsx_check_odr():
>    odr_table = &sensor->hw->settings->odr_table[sensor->id];
>
> and in function st_lsm6dsx_set_odr():
>    reg = &hw->settings->odr_table[ref_sensor->id].reg;
>
> To fix this possible array overflow, ref_sensor->id should be checked
> first. If it is greater than or equal to 2, the function
> st_lsm6dsx_set_odr() returns -EINVAL.
>
> Reported-by: TOTE Robot <oslab@...nghua.edu.cn>
> Signed-off-by: Teng Qi <starmiku1207184332@...il.com>

Hi,

Thanks for the patch, good catch. But there are a few things to improve 
here, the goal should not be to silence the output of your checker, but 
to write good code.

Let's start with ST_LSM6DSX_ID_MAX. As the name suggests this is an 
entry at the end of the enum that is used to get the size of the enum. 
But its value itself is never assigned to any variable of that type. 
This is a very common pattern. So strictly speaking there is no bug, 
since the out-of-range value of ST_LSM6DSX_ID_MAX is never used.

The other thing is that we usually don't want to hardcode range checks 
for array sizes with integer literals, but rather use ARRAY_SIZE() 
instead. This makes sure that the code stays correct when the array size 
changes.

If you really wanted to modify the code sot that your code checker does 
not detect a false positive I'd modify the switch statement to 
explicitly handle ST_LSM6DSX_ID_GYRO and ST_LSM6DSX_ID_ACCEL and the 
return an error for the default case with a comment that default case 
should never occur.

- Lars

> ---
>   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index db45f1fc0b81..edf5d33dd256 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1308,6 +1308,10 @@ st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u32 req_odr)
>   		break;
>   	}
>   
> +	if (ref_sensor->id >= 2) {
> +		return -EINVAL;
> +	}
> +
>   	if (req_odr > 0) {
>   		err = st_lsm6dsx_check_odr(ref_sensor, req_odr, &val);
>   		if (err < 0)