Re: [PATCH v2 2/4] md/raid10: fix overflow in safe_delay_store

[Li Nan]

在 2023/5/6 10:00, Yu Kuai 写道:
> Hi,
>
> 在 2023/05/06 9:23, linan666@xxxxxxxxxxxxxxx 写道:
> > From: Li Nan <linan122@xxxxxxxxxx>
> >
> > There is no input check when echo md/safe_mode_delay, and overflow will
> > occur. There is risk of overflow in strict_strtoul_scaled(), too. Fix it
> > by using kstrtoul instead of parsing word one by one.
>
> Other than some nits below, this patch looks good to me,
> feel free to add:
>
> Reviewed-by: Yu Kuai <yukuai3@xxxxxxxxxx>
> > Fixes: 72e02075a33f ("md: factor out parsing of fixed-point numbers")
> > Signed-off-by: Li Nan <linan122@xxxxxxxxxx>
> > ---
> >   drivers/md/md.c | 70 ++++++++++++++++++++++++++++++++-----------------
> >   1 file changed, 46 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 8e344b4b3444..fd5c3babcd6d 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -3767,52 +3767,74 @@ static int analyze_sbs(struct mddev *mddev)
> >    */
> >   int strict_strtoul_scaled(const char *cp, unsigned long *res, int 
> > scale)
> >   {
> > -    unsigned long result = 0;
> > -    long decimals = -1;
> > -    while (isdigit(*cp) || (*cp == '.' && decimals < 0)) {
> > -        if (*cp == '.')
> > -            decimals = 0;
> > -        else if (decimals < scale) {
> > -            unsigned int value;
> > -            value = *cp - '0';
> > -            result = result * 10 + value;
> > -            if (decimals >= 0)
> > -                decimals++;
> > -        }
> > -        cp++;
> > -    }
> > -    if (*cp == '\n')
> > -        cp++;
> > -    if (*cp)
> > +    unsigned long result = 0, decimals = 0;
> > +    char *pos, *str;
> > +    int rv;
> > +
> > +    str = kmemdup_nul(cp, strlen(cp), GFP_KERNEL);
> > +    if (!str)
> > +        return -ENOMEM;
> > +    pos = strchr(str, '.');
> > +    if (pos) {
> > +        int cnt = scale;
> > +
> > +        *pos = '\0';
> > +        while (isdigit(*(++pos))) {
> > +            if (cnt) {
> > +                decimals = decimals * 10 + *pos - '0';
> > +                cnt--;
> > +            }
> > +        }
> > +        if (*pos == '\n')
> > +            pos++;
> > +        if (*pos) {
> > +            kfree(str);
> > +            return -EINVAL;
> > +        }
> > +        decimals *= int_pow(10, cnt);
> > +    }
> > +
> > +    rv = kstrtoul(str, 10, &result);
> > +    kfree(str);
> > +    if (rv)
> > +        return rv;
> > +
> > +    if (result > (ULONG_MAX - decimals) / (unsigned int)int_pow(10, 
> > scale))
>
> This is correct, I guess the reason to use unsigned int is that u64/u64
> will compile error in some 32-bit architecture. It's better to use
> div64_u64() here.
>
> >           return -EINVAL;
> > -    if (decimals < 0)
> > -        decimals = 0;
> > -    *res = result * int_pow(10, scale - decimals);
> > -    return 0;
> > +    *res = result * int_pow(10, scale) + decimals;
> > +
> > +    return rv;
> >   }
> >   static ssize_t
> >   safe_delay_show(struct mddev *mddev, char *page)
> >   {
> > -    int msec = (mddev->safemode_delay*1000)/HZ;
> > -    return sprintf(page, "%d.%03d\n", msec/1000, msec%1000);
> > +    unsigned int msec = ((unsigned long)mddev->safemode_delay*1000)/HZ;
> > +
> > +    return sprintf(page, "%u.%03u\n", msec/1000, msec%1000);
> >   }
> >   static ssize_t
> >   safe_delay_store(struct mddev *mddev, const char *cbuf, size_t len)
> >   {
> >       unsigned long msec;
> > +    int ret;
> >       if (mddev_is_clustered(mddev)) {
> >           pr_warn("md: Safemode is disabled for clustered mode\n");
> >           return -EINVAL;
> >       }
> > -    if (strict_strtoul_scaled(cbuf, &msec, 3) < 0)
> > +    ret = strict_strtoul_scaled(cbuf, &msec, 3);
> > +    if (ret < 0)
> > +        return ret;
> > +    if (msec > UINT_MAX)
> >           return -EINVAL;
> > +
> >       if (msec == 0)
> >           mddev->safemode_delay = 0;
> >       else {
> >           unsigned long old_delay = mddev->safemode_delay;
> > +        /* HZ <= 1000, so new_delay < UINT_MAX, too */
>
> new_delay <= UNIT_MAX
>
> >           unsigned long new_delay = (msec*HZ)/1000;
>
> There is no need do declare them as 'unsigned long', you can use
> 'unsigned int' directly now.
>
> And you can also use DIV64_U64_ROUND_UP() directly here.

I will fix it in v3.

> Thanks,
> Kuai
> >           if (new_delay == 0)
>
> .

--
Thanks,
Nan