Re: [PATCH] xattr: Additional maximum size check

["J. Bruce Fields" <bfields@redhat.com>]

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
>