[SRU][Xenial][Bionic][PATCH 0/1] getxattr: use correct xattr length

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][Xenial][Bionic][PATCH 0/1] getxattr: use correct xattr length

Joseph Salisbury-3
BugLink: https://bugs.launchpad.net/bugs/1789746

== SRU Justification ==
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 has already been applied to Cosmic master-next.

== Fix ==
82c9a927bc5d ("getxattr: use correct xattr length")

== Regression Potential ==
Low.  One liner that passes the actual length of the xattr as returned by
vfs_getxattr() down.

== Test Case ==
A test kernel was built with this patch and tested by the original bug reporter.
The bug reporter states the test kernel resolved the bug.


Christian Brauner (1):
  getxattr: use correct xattr length

 fs/xattr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Xenial][Bionic][PATCH 1/1] getxattr: use correct xattr length

Joseph Salisbury-3
From: Christian Brauner <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1789746

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

Cc: [hidden email]
Fixes: 2f6f0654ab61 ("userns: Convert vfs posix_acl support to use kuids and kgids")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=199945
Reported-by: Colin Watson <[hidden email]>
Signed-off-by: Christian Brauner <[hidden email]>
Acked-by: Serge Hallyn <[hidden email]>
Signed-off-by: Eric W. Biederman <[hidden email]>
(cherry picked from commit 82c9a927bc5df6e06b72d206d24a9d10cced4eb5)
Signed-off-by: Joseph Salisbury <[hidden email]>
---
 fs/xattr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index f3c0b17..7b90d3c 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -467,7 +467,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.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][Xenial][Bionic][PATCH 1/1] getxattr: use correct xattr length

Kleber Souza
On 09/21/18 12:02, Joseph Salisbury wrote:

> From: Christian Brauner <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1789746
>
> 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
>
> Cc: [hidden email]
> Fixes: 2f6f0654ab61 ("userns: Convert vfs posix_acl support to use kuids and kgids")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199945
> Reported-by: Colin Watson <[hidden email]>
> Signed-off-by: Christian Brauner <[hidden email]>
> Acked-by: Serge Hallyn <[hidden email]>
> Signed-off-by: Eric W. Biederman <[hidden email]>
> (cherry picked from commit 82c9a927bc5df6e06b72d206d24a9d10cced4eb5)
> Signed-off-by: Joseph Salisbury <[hidden email]>

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

> ---
>  fs/xattr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index f3c0b17..7b90d3c 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -467,7 +467,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) {
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][Xenial][Bionic][PATCH 1/1] getxattr: use correct xattr length

Stefan Bader-2
In reply to this post by Joseph Salisbury-3
On 21.09.2018 12:02, Joseph Salisbury wrote:

> From: Christian Brauner <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1789746
>
> 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
>
> Cc: [hidden email]
> Fixes: 2f6f0654ab61 ("userns: Convert vfs posix_acl support to use kuids and kgids")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199945
> Reported-by: Colin Watson <[hidden email]>
> Signed-off-by: Christian Brauner <[hidden email]>
> Acked-by: Serge Hallyn <[hidden email]>
> Signed-off-by: Eric W. Biederman <[hidden email]>
> (cherry picked from commit 82c9a927bc5df6e06b72d206d24a9d10cced4eb5)
> Signed-off-by: Joseph Salisbury <[hidden email]>
Acked-by: Stefan Bader <[hidden email]>

> ---
>  fs/xattr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index f3c0b17..7b90d3c 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -467,7 +467,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) {
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

APPLIED[B]: [SRU][Xenial][Bionic][PATCH 0/1] getxattr: use correct xattr length

Stefan Bader-2
In reply to this post by Joseph Salisbury-3
On 21.09.2018 12:02, Joseph Salisbury wrote:

> BugLink: https://bugs.launchpad.net/bugs/1789746
>
> == SRU Justification ==
> 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 has already been applied to Cosmic master-next.
>
> == Fix ==
> 82c9a927bc5d ("getxattr: use correct xattr length")
>
> == Regression Potential ==
> Low.  One liner that passes the actual length of the xattr as returned by
> vfs_getxattr() down.
>
> == Test Case ==
> A test kernel was built with this patch and tested by the original bug reporter.
> The bug reporter states the test kernel resolved the bug.
>
>
> Christian Brauner (1):
>   getxattr: use correct xattr length
>
>  fs/xattr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
Applied to bionix/master-next. Thanks.

-Stefan


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

NAK[Xenial]: [SRU][Xenial][Bionic][PATCH 1/1] getxattr: use correct xattr length

Kleber Souza
In reply to this post by Joseph Salisbury-3
On 09/21/18 12:02, Joseph Salisbury wrote:

> From: Christian Brauner <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1789746
>
> 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
>
> Cc: [hidden email]
> Fixes: 2f6f0654ab61 ("userns: Convert vfs posix_acl support to use kuids and kgids")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199945
> Reported-by: Colin Watson <[hidden email]>
> Signed-off-by: Christian Brauner <[hidden email]>
> Acked-by: Serge Hallyn <[hidden email]>
> Signed-off-by: Eric W. Biederman <[hidden email]>
> (cherry picked from commit 82c9a927bc5df6e06b72d206d24a9d10cced4eb5)
> Signed-off-by: Joseph Salisbury <[hidden email]>
> ---
>  fs/xattr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index f3c0b17..7b90d3c 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -467,7 +467,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) {
>

The patch has already been applied to xenial/master-next as part of LP:
#1792419 (Xenial update to 4.4.155 stable release). So skipping it...


Thanks,
Kleber

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team