[PATCH] xattr: Additional maximum size check

[PATCH] xattr: Additional maximum size check

[Andreas Gruenbacher]

When querying for the buffer size required, a filesystem's getxattr
xattr handler or listxattr iop may return a value bigger than the
maximum size limit.  However, the VFS will never return oversize
buffers, so cast such values to -E2BIG.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/xattr.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 7e3317c..c19a163 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -537,16 +537,18 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
 	}
 
 	error = vfs_getxattr(d, kname, kvalue, size);
+	if (error > XATTR_SIZE_MAX ||
+	    (error == -ERANGE && size >= XATTR_SIZE_MAX)) {
+		/* The file system tried to returned a value bigger
+		   than XATTR_SIZE_MAX bytes. Not possible. */
+		error = -E2BIG;
+	}
 	if (error > 0) {
 		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
 		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
 			posix_acl_fix_xattr_to_user(kvalue, size);
 		if (size && copy_to_user(value, kvalue, error))
 			error = -EFAULT;
-	} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
-		/* The file system tried to returned a value bigger
-		   than XATTR_SIZE_MAX bytes. Not possible. */
-		error = -E2BIG;
 	}
 
 	kvfree(kvalue);
@@ -620,14 +622,16 @@ listxattr(struct dentry *d, char __user *list, size_t size)
 	}
 
 	error = vfs_listxattr(d, klist, size);
-	if (error > 0) {
-		if (size && copy_to_user(list, klist, error))
-			error = -EFAULT;
-	} else if (error == -ERANGE && size >= XATTR_LIST_MAX) {
+	if (error > XATTR_LIST_MAX ||
+	    (error == -ERANGE && size >= XATTR_LIST_MAX)) {
 		/* The file system tried to returned a list bigger
 		   than XATTR_LIST_MAX bytes. Not possible. */
 		error = -E2BIG;
 	}
+	if (error > 0) {
+		if (size && copy_to_user(list, klist, error))
+			error = -EFAULT;
+	}
 
 	kvfree(klist);
 
-- 
2.7.4

Re: [PATCH] xattr: Additional maximum size check

[J. Bruce Fields]

On Thu, Feb 23, 2017 at 03:52:14PM +0100, Andreas Gruenbacher wrote:
> When querying for the buffer size required, a filesystem's getxattr
> xattr handler or listxattr iop may return a value bigger than the
> maximum size limit.  However, the VFS will never return oversize
> buffers, so cast such values to -E2BIG.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/xattr.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 7e3317c..c19a163 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -537,16 +537,18 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
>  	}
>  
>  	error = vfs_getxattr(d, kname, kvalue, size);

We have this just above:

	if (size) {
		if (size > XATTR_SIZE_MAX)
			size = XATTR_SIZE_MAX;

So this test:

> +	if (error > XATTR_SIZE_MAX ||
> +	    (error == -ERANGE && size >= XATTR_SIZE_MAX)) {
> +		/* The file system tried to returned a value bigger
> +		   than XATTR_SIZE_MAX bytes. Not possible. */
> +		error = -E2BIG;
> +	}

is equivalent to:

	if (error > XATTR_SIZE_MAX ||
	    (error = -ERANGE && size == XATTR_SIZE_MAX))

That's kind of a subtle case.  I guess the idea is that ERANGE means
"the xattr value doesn't fit into the buffer you gave me", and EF2BIG
means "the xattr value couldn't fit in any buffer you could possible
give me".  And if the filesystem isn't satisfied even with a
maximum-sized buffer then clearly we're in the second case.  OK, got it.
Still, this is a little subtle.  I don't see EF2BIG in my copy of
getxattr(2).

Anyway, feel free to add

	Reviewed-by: J. Bruce Fields <bfields@redhat.com>

but I wouldn't say no to more documentation.

--b.

>  	if (error > 0) {
>  		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
>  		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
>  			posix_acl_fix_xattr_to_user(kvalue, size);
>  		if (size && copy_to_user(value, kvalue, error))
>  			error = -EFAULT;
> -	} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
> -		/* The file system tried to returned a value bigger
> -		   than XATTR_SIZE_MAX bytes. Not possible. */
> -		error = -E2BIG;
>  	}
>  
>  	kvfree(kvalue);
> @@ -620,14 +622,16 @@ listxattr(struct dentry *d, char __user *list, size_t size)
>  	}
>  
>  	error = vfs_listxattr(d, klist, size);
> -	if (error > 0) {
> -		if (size && copy_to_user(list, klist, error))
> -			error = -EFAULT;
> -	} else if (error == -ERANGE && size >= XATTR_LIST_MAX) {
> +	if (error > XATTR_LIST_MAX ||
> +	    (error == -ERANGE && size >= XATTR_LIST_MAX)) {
>  		/* The file system tried to returned a list bigger
>  		   than XATTR_LIST_MAX bytes. Not possible. */
>  		error = -E2BIG;
>  	}
> +	if (error > 0) {
> +		if (size && copy_to_user(list, klist, error))
> +			error = -EFAULT;
> +	}
>  
>  	kvfree(klist);
>  
> -- 
> 2.7.4
> 

Re: [PATCH] xattr: Additional maximum size check

[Andreas Gruenbacher]

On Thu, Feb 23, 2017 at 9:34 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> On Thu, Feb 23, 2017 at 03:52:14PM +0100, Andreas Gruenbacher wrote:
>> When querying for the buffer size required, a filesystem's getxattr
>> xattr handler or listxattr iop may return a value bigger than the
>> maximum size limit.  However, the VFS will never return oversize
>> buffers, so cast such values to -E2BIG.
>>
>> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
>> ---
>>  fs/xattr.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/xattr.c b/fs/xattr.c
>> index 7e3317c..c19a163 100644
>> --- a/fs/xattr.c
>> +++ b/fs/xattr.c
>> @@ -537,16 +537,18 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
>>       }
>>
>>       error = vfs_getxattr(d, kname, kvalue, size);
>
> We have this just above:
>
>         if (size) {
>                 if (size > XATTR_SIZE_MAX)
>                         size = XATTR_SIZE_MAX;
>
> So this test:
>
>> +     if (error > XATTR_SIZE_MAX ||
>> +         (error == -ERANGE && size >= XATTR_SIZE_MAX)) {
>> +             /* The file system tried to returned a value bigger
>> +                than XATTR_SIZE_MAX bytes. Not possible. */
>> +             error = -E2BIG;
>> +     }
>
> is equivalent to:
>
>         if (error > XATTR_SIZE_MAX ||
>             (error = -ERANGE && size == XATTR_SIZE_MAX))
>
> That's kind of a subtle case.

True.

> I guess the idea is that ERANGE means
> "the xattr value doesn't fit into the buffer you gave me", and E2BIG
> means "the xattr value couldn't fit in any buffer you could possible
> give me".  And if the filesystem isn't satisfied even with a
> maximum-sized buffer then clearly we're in the second case.  OK, got it.
> Still, this is a little subtle.  I don't see EF2BIG in my copy of
> getxattr(2).

It's not in any of my copies of getxattr(2) either yet, but it will be soon :)

> Anyway, feel free to add
>
>         Reviewed-by: J. Bruce Fields <bfields@redhat.com>
>
> but I wouldn't say no to more documentation.

Thanks,
Andreas