Hey,
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.
I think that the analysis that 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() is correct.
So this commit passes the actual length of the xattr as returned by
vfs_getxattr() down.
It might be worth considering making posix_acl_fix_xattr_userns() and
posix_acl_fix_xattr_to_user() return errors. Such that e.g.
posix_acl_xattr_count() errors can be caught in getxattr(). If that's
something we want I can send a follow-up patch.
More details of this can be found in
https://bugzilla.kernel.org/show_bug.cgi?id=199945
Thanks!
Christian
Christian Brauner (1):
getxattr: use correct xattr length
fs/xattr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
2.17.0
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 <[email protected]>
Signed-off-by: Christian Brauner <[email protected]>
---
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 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 <[email protected]>
> Signed-off-by: Christian Brauner <[email protected]>
D'oh, sorry, I thought I had replied to this!
Acked-by: Serge Hallyn <[email protected]>
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 <[email protected]>
> > Signed-off-by: Christian Brauner <[email protected]>
>
> D'oh, sorry, I thought I had replied to this!
No problem at all. :)
Thanks!
Christian
>
> Acked-by: Serge Hallyn <[email protected]>
>
> 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 <[email protected]>
> > Signed-off-by: Christian Brauner <[email protected]>
>
> D'oh, sorry, I thought I had replied to this!
>
> Acked-by: Serge Hallyn <[email protected]>
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 ([email protected]):
> 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 <[email protected]>
> > > Signed-off-by: Christian Brauner <[email protected]>
> >
> > D'oh, sorry, I thought I had replied to this!
> >
> > Acked-by: Serge Hallyn <[email protected]>
>
> 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