| Message ID | 20180607114348.23667-2-christian@brauner.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@ubuntu.com> > Signed-off-by: Christian Brauner <christian@brauner.io> D'oh, sorry, I thought I had replied to this! Acked-by: Serge Hallyn <serge@hallyn.com> 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
On Wed, Jun 13, 2018 at 10:45:37AM -0500, Serge Hallyn wrote: > 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@ubuntu.com> > > Signed-off-by: Christian Brauner <christian@brauner.io> > > D'oh, sorry, I thought I had replied to this! No problem at all. :) Thanks! Christian > > Acked-by: Serge Hallyn <serge@hallyn.com> > > 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
On Wed, Jun 13, 2018 at 10:45:37AM -0500, Serge Hallyn wrote: > 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@ubuntu.com> > > Signed-off-by: Christian Brauner <christian@brauner.io> > > D'oh, sorry, I thought I had replied to this! > > Acked-by: Serge Hallyn <serge@hallyn.com> Should this still land as a bugfix in 4.18? Christian > > 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
Quoting Christian Brauner (christian.brauner@canonical.com): > On Wed, Jun 13, 2018 at 10:45:37AM -0500, Serge Hallyn wrote: > > 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@ubuntu.com> > > > Signed-off-by: Christian Brauner <christian@brauner.io> > > > > D'oh, sorry, I thought I had replied to this! > > > > Acked-by: Serge Hallyn <serge@hallyn.com> > > Should this still land as a bugfix in 4.18? Eric, do you mind picking this up? > Christian > > > > > 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
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) {
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@ubuntu.com> Signed-off-by: Christian Brauner <christian@brauner.io> --- fs/xattr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)