[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cc4cfcf6-54cf-a271-4082-687e8ba5c1c5@huaweicloud.com>
Date: Thu, 27 Apr 2023 19:16:13 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: linan666@...weicloud.com, song@...nel.org, neilb@...e.de,
Rob.Becker@...erbed.com
Cc: linux-raid@...r.kernel.org, linux-kernel@...r.kernel.org,
linan122@...wei.com, yi.zhang@...wei.com, houtao1@...wei.com,
yangerkun@...wei.com, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH 2/3] md/raid10: fix overflow in safe_delay_store
Hi,
Other than some nits below, this patch looks good to me.
在 2023/04/27 16:56, linan666@...weicloud.com 写道:
> From: Li Nan <linan122@...wei.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@...wei.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)
>
Powered by blists - more mailing lists