Re: [PATCH 1/1] getxattr: use correct xattr length

[Serge E. Hallyn]

On Thu, Jun 07, 2018 at 01:43:48PM +0200, Christian Brauner wrote:
> When running in a container with a user namespace, if you call getxattr
> with name = "system.posix_acl_access" and size % 8 != 4, then getxattr
> silently skips the user namespace fixup that it normally does resulting in
> un-fixed-up data being returned.
> This is caused by posix_acl_fix_xattr_to_user() being passed the total
> buffer size and not the actual size of the xattr as returned by
> vfs_getxattr().
> This commit passes the actual length of the xattr as returned by
> vfs_getxattr() down.
> 
> A reproducer for the issue is:
> 
>   touch acl_posix
> 
>   setfacl -m user:0:rwx acl_posix
> 
> and the compile:
> 
>   #define _GNU_SOURCE
>   #include <errno.h>
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <string.h>
>   #include <sys/types.h>
>   #include <unistd.h>
>   #include <attr/xattr.h>
> 
>   /* Run in user namespace with nsuid 0 mapped to uid != 0 on the host. */
>   int main(int argc, void **argv)
>   {
>           ssize_t ret1, ret2;
>           char buf1[128], buf2[132];
>           int fret = EXIT_SUCCESS;
>           char *file;
> 
>           if (argc < 2) {
>                   fprintf(stderr,
>                           "Please specify a file with "
>                           "\"system.posix_acl_access\" permissions set\n");
>                   _exit(EXIT_FAILURE);
>           }
>           file = argv[1];
> 
>           ret1 = getxattr(file, "system.posix_acl_access",
>                           buf1, sizeof(buf1));
>           if (ret1 < 0) {
>                   fprintf(stderr, "%s - Failed to retrieve "
>                                   "\"system.posix_acl_access\" "
>                                   "from \"%s\"\n", strerror(errno), file);
>                   _exit(EXIT_FAILURE);
>           }
> 
>           ret2 = getxattr(file, "system.posix_acl_access",
>                           buf2, sizeof(buf2));
>           if (ret2 < 0) {
>                   fprintf(stderr, "%s - Failed to retrieve "
>                                   "\"system.posix_acl_access\" "
>                                   "from \"%s\"\n", strerror(errno), file);
>                   _exit(EXIT_FAILURE);
>           }
> 
>           if (ret1 != ret2) {
>                   fprintf(stderr, "The value of \"system.posix_acl_"
>                                   "access\" for file \"%s\" changed "
>                                   "between two successive calls\n", file);
>                   _exit(EXIT_FAILURE);
>           }
> 
>           for (ssize_t i = 0; i < ret2; i++) {
>                   if (buf1[i] == buf2[i])
>                           continue;
> 
>                   fprintf(stderr,
>                           "Unexpected different in byte %zd: "
>                           "%02x != %02x\n", i, buf1[i], buf2[i]);
>                   fret = EXIT_FAILURE;
>           }
> 
>           if (fret == EXIT_SUCCESS)
>                   fprintf(stderr, "Test passed\n");
>           else
>                   fprintf(stderr, "Test failed\n");
> 
>           _exit(fret);
>   }
> and run:
> 
>   ./tester acl_posix
> 
> On a non-fixed up kernel this should return something like:
> 
>   root@c1:/# ./t
>   Unexpected different in byte 16: ffffffa0 != 00
>   Unexpected different in byte 17: ffffff86 != 00
>   Unexpected different in byte 18: 01 != 00
> 
> and on a fixed kernel:
> 
>   root@c1:~# ./t
>   Test passed
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199945
> Reported-by: Colin Watson <cjwatson@xxxxxxxxxx>
> Signed-off-by: Christian Brauner <christian@xxxxxxxxxx>

D'oh, sorry, I thought I had replied to this!

Acked-by: Serge Hallyn <serge@xxxxxxxxxx>

thanks,
Serge

> ---
>  fs/xattr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index f9cb1db187b7..1bee74682513 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -539,7 +539,7 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
>  	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);
> +			posix_acl_fix_xattr_to_user(kvalue, error);
>  		if (size && copy_to_user(value, kvalue, error))
>  			error = -EFAULT;
>  	} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
> -- 
> 2.17.0