diff mbox series

[2/3] md/raid10: fix overflow in safe_delay_store

Message ID 20230427085612.1346752-3-linan666@huaweicloud.com
State New
Headers
Series md: bugfix of writing raid sysfs |

Commit Message

Li Nan April 27, 2023, 8:56 a.m. UTC 
  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
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(-)

Comments

Yu Kuai April 27, 2023, 11:16 a.m. UTC | #1 
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)
>
Li Nan May 5, 2023, 7:07 a.m. UTC | #2 
Thank for suggestion, It will be optimized in v2.
diff mbox series

Patch

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)