On Mon, 13 Mar 2023 15:04:28 +0300
Dan Carpenter <error27@xxxxxxxxx> 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@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?
>
> 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
>
>
>
