Re: [PATCH v2 2/4] md/raid10: fix overflow in safe_delay_store
From: Yu Kuai <hidden>
Date: 2023-05-06 02:00:46
Also in:
lkml
Hi, 在 2023/05/06 9:23, linan666@huaweicloud.com 写道:
From: Li Nan <redacted> 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 <redacted>
quoted hunk ↗ jump to hunk
Fixes: 72e02075a33f ("md: factor out parsing of fixed-point numbers") Signed-off-by: Li Nan <redacted> --- 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)