| Message ID | 94939714-a232-4107-8741-8867038b03ae@kili.mountain (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: magn: bmc150: add a lower bounds in bmc150_magn_write_raw() | expand |
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);
On Sun, Mar 12, 2023 at 02:45:51PM +0000, Jonathan Cameron wrote: > 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? Okay, fair enough. The upper bounds is required and the lower bounds is not. However, passing negatives is still not best practice and I feel like it wasn't intentional here. Let me resend the commit, but with a different commit message that doesn't say the upper bound is not required. The Smatch warning feels intuitively correct. If you're going to have an upper bounds check then you need to have a lower bounds check to prevent negative values. In practice it works pretty well. The only major issue with this check is that sometimes Smatch thinks a variable can be negative when it cannot. This patch is an example where passing a negative is harmless and I had a similar warning last week where it was passing a negative param was harmless as well. The parameter was used as loop limit: for (i = 0; i < param; i++) { It's a no-op since param is negative, but all all it needs is for someone declare the iterator as "unsigned int i;" and then it becomes a memory corruption issue. So occasionally passing negatives is harmless but mostly it's bad. regards, dan carpenter
On Mon, 13 Mar 2023 15:04:28 +0300 Dan Carpenter <error27@gmail.com> wrote: > On Sun, Mar 12, 2023 at 02:45:51PM +0000, Jonathan Cameron wrote: > > 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? > > Okay, fair enough. The upper bounds is required and the lower bounds is > not. > > However, passing negatives is still not best practice and I feel like it > wasn't intentional here. Let me resend the commit, but with a different > commit message that doesn't say the upper bound is not required. That works for me. > > The Smatch warning feels intuitively correct. If you're going to have > an upper bounds check then you need to have a lower bounds check to > prevent negative values. In practice it works pretty well. The only > major issue with this check is that sometimes Smatch thinks a variable > can be negative when it cannot. > > This patch is an example where passing a negative is harmless and I had > a similar warning last week where it was passing a negative param was > harmless as well. The parameter was used as loop limit: > > for (i = 0; i < param; i++) { > > It's a no-op since param is negative, but all all it needs is for > someone declare the iterator as "unsigned int i;" and then it becomes > a memory corruption issue. > > So occasionally passing negatives is harmless but mostly it's bad. Agreed. > > regards, > dan carpenter > > >
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);
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> --- drivers/iio/magnetometer/bmc150_magn.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)