On Wed, 8 Mar 2023 12:12:37 +0300
Dan Carpenter <error27@xxxxxxxxx> wrote:
> The "val" variable comes from the user via iio_write_channel_info().
> This code puts an upper bound on "val" but it doesn't check for
> negatives so Smatch complains. I don't think either the bounds
> checking is really required, but it's just good to be conservative.
>
> Fixes: 5990dc970367 ("iio: magn: bmc150_magn: add oversampling ratio")
> Signed-off-by: Dan Carpenter <error27@xxxxxxxxx>
Hi Dan,
I think this is more complex than it initially appears.
bmc150_magn_set_odr() matches against a table of possible value
(precise matching) and as such you'd assume neither check is necessary.
However, for a given configuration not all values in that table can
actually be set due to max_odr actually changing depending on other settings.
My immediate thought was "why not push this check into bmc150_magn_set_odr()"
where this will be more obvious. Turns out that max_odr isn't available until
later in bmc150_magn_init() than the initial call of bmc150_magn_set_odr()
Whilst I 'think' you could move that around so that max_odr was set, that's not quite
obvious enough for me to want to do it without testing the result.
So question becomes is it wroth adding the val < 0 check here.
My gut feeling is that actually makes it more confusing because we are checking
something that doesn't restrict the later results alongside something that does.
Am I missing something, or was smatch just being overly careful?
Jonathan
> ---
> drivers/iio/magnetometer/bmc150_magn.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index 06d5a1ef1fbd..c625416b8bcf 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -537,7 +537,7 @@ static int bmc150_magn_write_raw(struct iio_dev *indio_dev,
>
> switch (mask) {
> case IIO_CHAN_INFO_SAMP_FREQ:
> - if (val > data->max_odr)
> + if (val < 0 || val > data->max_odr)
> return -EINVAL;
> mutex_lock(&data->mutex);
> ret = bmc150_magn_set_odr(data, val);
