Re: [PATCH] iio: magn: bmc150: add a lower bounds in bmc150_magn_write_raw()

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 8 Mar 2023 12:12:37 +0300
Dan Carpenter <error27@gmail.com> 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@gmail.com>

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);