[PATCH v2] libfs: fix error cast of negative value in simple_attr_write()

[PATCH v2] libfs: fix error cast of negative value in simple_attr_write()

[Yicong Yang]

The attr->set() receive a value of u64, but simple_strtoll() is used
for doing the conversion. It will lead to the error cast if user inputs
a negative value.

Use kstrtoull() instead of simple_strtoll() to convert a string got
from the user to an unsigned value. The former will return '-EINVAL' if
it gets a negetive value, but the latter can't handle the situation
correctly.

Fixes: f7b88631a897 ("fs/libfs.c: fix simple_attr_write() on 32bit machines")
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
Change since v1:
- address the compile warning for non-64 bit platform

 fs/libfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index fc34361..3a0d99c 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -977,7 +977,9 @@ ssize_t simple_attr_write(struct file *file, const char __user *buf,
 		goto out;
 
 	attr->set_buf[size] = '\0';
-	val = simple_strtoll(attr->set_buf, NULL, 0);
+	ret = kstrtoull(attr->set_buf, 0, (unsigned long long *)&val);
+	if (ret)
+		goto out;
 	ret = attr->set(attr->data, val);
 	if (ret == 0)
 		ret = len; /* on success, claim we got the whole input */
-- 
2.8.1

RE: [PATCH v2] libfs: fix error cast of negative value in simple_attr_write()

[David Laight]

From: Yicong Yang
> Sent: 13 November 2020 09:56
> The attr->set() receive a value of u64, but simple_strtoll() is used
> for doing the conversion. It will lead to the error cast if user inputs
> a negative value.
> 
> Use kstrtoull() instead of simple_strtoll() to convert a string got
> from the user to an unsigned value. The former will return '-EINVAL' if
> it gets a negetive value, but the latter can't handle the situation
> correctly.
> 
> Fixes: f7b88631a897 ("fs/libfs.c: fix simple_attr_write() on 32bit machines")
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
> Change since v1:
> - address the compile warning for non-64 bit platform
> 
>  fs/libfs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index fc34361..3a0d99c 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -977,7 +977,9 @@ ssize_t simple_attr_write(struct file *file, const char __user *buf,
>  		goto out;
> 
>  	attr->set_buf[size] = '\0';
> -	val = simple_strtoll(attr->set_buf, NULL, 0);
> +	ret = kstrtoull(attr->set_buf, 0, (unsigned long long *)&val);

That cast is horrid.
Casting 'pointer to integer' types is just asking for trouble.
You either need to change the type of 'val' or use an
intermediary variable of the correct type.

	David

> +	if (ret)
> +		goto out;
>  	ret = attr->set(attr->data, val);
>  	if (ret == 0)
>  		ret = len; /* on success, claim we got the whole input */
> --
> 2.8.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2] libfs: fix error cast of negative value in simple_attr_write()

[Al Viro]

On Fri, Nov 13, 2020 at 05:56:09PM +0800, Yicong Yang wrote:
> The attr->set() receive a value of u64, but simple_strtoll() is used
> for doing the conversion. It will lead to the error cast if user inputs
> a negative value.
> 
> Use kstrtoull() instead of simple_strtoll() to convert a string got
> from the user to an unsigned value. The former will return '-EINVAL' if
> it gets a negetive value, but the latter can't handle the situation
> correctly.
> 
> Fixes: f7b88631a897 ("fs/libfs.c: fix simple_attr_write() on 32bit machines")
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
> Change since v1:
> - address the compile warning for non-64 bit platform
> 
>  fs/libfs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index fc34361..3a0d99c 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -977,7 +977,9 @@ ssize_t simple_attr_write(struct file *file, const char __user *buf,
>  		goto out;
>  
>  	attr->set_buf[size] = '\0';
> -	val = simple_strtoll(attr->set_buf, NULL, 0);
> +	ret = kstrtoull(attr->set_buf, 0, (unsigned long long *)&val);
> +	if (ret)
> +		goto out;
>  	ret = attr->set(attr->data, val);
>  	if (ret == 0)
>  		ret = len; /* on success, claim we got the whole input */

Make val unsigned long long instead.