| Message ID | 20230506012315.3370489-3-linan666@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | md: bugfix of writing raid sysfs | expand |
Hi, 在 2023/05/06 9:23, linan666@huaweicloud.com 写道: > From: Li Nan <linan122@huawei.com> > > 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@huawei.com> > > Fixes: 72e02075a33f ("md: factor out parsing of fixed-point numbers") > Signed-off-by: Li Nan <linan122@huawei.com> > --- > 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. Thanks, Kuai > > if (new_delay == 0) >
在 2023/5/6 10:00, Yu Kuai 写道: > Hi, > > 在 2023/05/06 9:23, linan666@huaweicloud.com 写道: >> From: Li Nan <linan122@huawei.com> >> >> 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@huawei.com> >> >> Fixes: 72e02075a33f ("md: factor out parsing of fixed-point numbers") >> Signed-off-by: Li Nan <linan122@huawei.com> >> --- >> 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) >> > > .
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)) 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 */ unsigned long new_delay = (msec*HZ)/1000; if (new_delay == 0)