* [PATCH] iio: imu: st_lsm6dsx: Fix an array overflow in st_lsm6dsx_set_odr()
@ 2021-10-10 9:12 Teng Qi
2021-10-10 10:03 ` Lars-Peter Clausen
0 siblings, 1 reply; 3+ messages in thread
From: Teng Qi @ 2021-10-10 9:12 UTC (permalink / raw)
To: lorenzo.bianconi83, jic23, lars
Cc: linux-iio, linux-kernel, islituo, baijiaju1990, Teng Qi, TOTE Robot
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@tsinghua.edu.cn>
Signed-off-by: Teng Qi <starmiku1207184332@gmail.com>
---
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)
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: Fix an array overflow in st_lsm6dsx_set_odr()
2021-10-10 9:12 [PATCH] iio: imu: st_lsm6dsx: Fix an array overflow in st_lsm6dsx_set_odr() Teng Qi
@ 2021-10-10 10:03 ` Lars-Peter Clausen
2021-10-11 9:47 ` Lorenzo Bianconi
0 siblings, 1 reply; 3+ messages in thread
From: Lars-Peter Clausen @ 2021-10-10 10:03 UTC (permalink / raw)
To: Teng Qi, lorenzo.bianconi83, jic23
Cc: linux-iio, linux-kernel, islituo, baijiaju1990, TOTE Robot
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@tsinghua.edu.cn>
> Signed-off-by: Teng Qi <starmiku1207184332@gmail.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)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: Fix an array overflow in st_lsm6dsx_set_odr()
2021-10-10 10:03 ` Lars-Peter Clausen
@ 2021-10-11 9:47 ` Lorenzo Bianconi
0 siblings, 0 replies; 3+ messages in thread
From: Lorenzo Bianconi @ 2021-10-11 9:47 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Teng Qi, lorenzo.bianconi83, jic23, linux-iio, linux-kernel,
islituo, baijiaju1990, TOTE Robot
[-- Attachment #1: Type: text/plain, Size: 2711 bytes --]
> 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@tsinghua.edu.cn>
> > Signed-off-by: Teng Qi <starmiku1207184332@gmail.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.
+1 :)
Regards,
Lorenzo
>
> - 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)
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-11 9:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-10 9:12 [PATCH] iio: imu: st_lsm6dsx: Fix an array overflow in st_lsm6dsx_set_odr() Teng Qi
2021-10-10 10:03 ` Lars-Peter Clausen
2021-10-11 9:47 ` Lorenzo Bianconi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.