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

[Jonathan Cameron <jic23@kernel.org>]

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