| Message ID | 20230427085612.1346752-3-linan666@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | md: bugfix of writing raid sysfs | expand |
Hi, Other than some nits below, this patch looks good to me. 在 2023/04/27 16:56, 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. Fixed typo: Fix > it by using kstrtoul instead of parsing word one by one. > > Fixes: 72e02075a33f ("md: factor out parsing of fixed-point numbers") > Signed-off-by: Li Nan <linan122@huawei.com> > --- > drivers/md/md.c | 66 ++++++++++++++++++++++++++++++++----------------- > 1 file changed, 43 insertions(+), 23 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index 8e344b4b3444..faffbd042925 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -3767,35 +3767,51 @@ 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) > @@ -3809,10 +3825,14 @@ safe_delay_store(struct mddev *mddev, const char *cbuf, size_t len) > > if (strict_strtoul_scaled(cbuf, &msec, 3) < 0) > return -EINVAL; strict_strtoul_scaled() can return -ENOMEM now. > + 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) >
Thank for suggestion, It will be optimized in v2.
diff --git a/drivers/md/md.c b/drivers/md/md.c index 8e344b4b3444..faffbd042925 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -3767,35 +3767,51 @@ 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) @@ -3809,10 +3825,14 @@ safe_delay_store(struct mddev *mddev, const char *cbuf, size_t len) if (strict_strtoul_scaled(cbuf, &msec, 3) < 0) return -EINVAL; + 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)