Re: [bug report] md: range check slot number when manually adding a spare.

[Dan Carpenter <error27@gmail.com>]

On Fri, Mar 03, 2023 at 10:09:50AM -0600, Roger Heflin wrote:
> On Fri, Mar 3, 2023 at 8:31 AM Dan Carpenter <error27@gmail.com> wrote:
> >
> > [ Ancient code, but you're still at the same email address...  -dan ]
> >
> > Hello NeilBrown,
> >
> > The patch ba1b41b6b4e3: "md: range check slot number when manually
> > adding a spare." from Jan 14, 2011, leads to the following Smatch
> > static checker warning:
> >
> > drivers/md/md.c:3170 slot_store() warn: no lower bound on 'slot'
> > drivers/md/md.c:3172 slot_store() warn: no lower bound on 'slot'
> > drivers/md/md.c:3190 slot_store() warn: no lower bound on 'slot'
> >
> > drivers/md/md.c
> >     3117 static ssize_t
> >     3118 slot_store(struct md_rdev *rdev, const char *buf, size_t len)
> >     3119 {
> >     3120         int slot;
> >     3121         int err;
> >     3122
> >     3123         if (test_bit(Journal, &rdev->flags))
> >     3124                 return -EBUSY;
> >     3125         if (strncmp(buf, "none", 4)==0)
> >     3126                 slot = -1;
> >     3127         else {
> >     3128                 err = kstrtouint(buf, 10, (unsigned int *)&slot);
> >
> > slot comes from the user.
> >
> >     3129                 if (err < 0)
> >     3130                         return err;
> >     3131         }
> 
> kstrtouint is string to unsigned int, it has this code:
> 
> if (tmp != (unsigned int)tmp)
> return -ERANGE;
> 
> And so will not return a negative number without an error, so 0 is the
> lower limit as enforced by the function.

Some of what you have written is correct, but your main conclusion is
wrong.  The kstrtouint() gives you unsigned ints.  If you take a very
large unsigned int and cast it to an int then you get a negative value
so the underflow issue is real.

Btw, in more modern code we would write:

	err = kstrtoint(buf, 10, &slot);
	if (err)
		return err;

It still has the underflow issue...  I have been wanting to say that and
resisted saying it because it was obvious to everyone.  However I am
only human and can only resist the temptation to point out the obvious
for so long.

regards,
dan carpenter