[PATCH] nptl: Properly inline setgroups syscall [BZ #26248]

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

[PATCH] nptl: Properly inline setgroups syscall [BZ #26248]

Sourceware - libc-alpha mailing list
nptl has

/* Opcodes and data types for communication with the signal handler to
   change user/group IDs.  */
struct xid_command
{
  int syscall_no;
  long int id[3];
  volatile int cntr;
  volatile int error;
};

 /* This must be last, otherwise the current thread might not have
     permissions to send SIGSETXID syscall to the other threads.  */
  result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
                                 cmdp->id[0], cmdp->id[1], cmdp->id[2]);

But the second argument of setgroups syscal is a pointer:

       int setgroups(size_t size, const gid_t *list);

But on x32, pointers passed to syscall must have pointer type so that they
will be zero-extended.

Add <setxid-internal.h> to define INTERNAL_SETXID_SYSCALL_NCS and use it,
instead of INTERNAL_SYSCALL_NCS, for SETXID syscalls.  X32 override it
with pointer type for setgroups.  A testcase is added and setgroups
returned with EFAULT when running as root without the fix.
---
 nptl/allocatestack.c                          |  4 +-
 nptl/nptl-init.c                              |  4 +-
 sysdeps/nptl/setxid-internal.h                | 21 +++++++
 sysdeps/unix/sysv/linux/x86_64/x32/Makefile   |  4 ++
 .../sysv/linux/x86_64/x32/setxid-internal.h   | 36 +++++++++++
 .../sysv/linux/x86_64/x32/tst-setgroups.c     | 62 +++++++++++++++++++
 6 files changed, 127 insertions(+), 4 deletions(-)
 create mode 100644 sysdeps/nptl/setxid-internal.h
 create mode 100644 sysdeps/unix/sysv/linux/x86_64/x32/setxid-internal.h
 create mode 100644 sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 4ae4b5a986..af5fc5f882 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -32,6 +32,7 @@
 #include <futex-internal.h>
 #include <kernel-features.h>
 #include <stack-aliasing.h>
+#include <setxid-internal.h>
 
 
 #ifndef NEED_SEPARATE_REGISTER_STACK
@@ -1159,8 +1160,7 @@ __nptl_setxid (struct xid_command *cmdp)
 
   /* This must be last, otherwise the current thread might not have
      permissions to send SIGSETXID syscall to the other threads.  */
-  result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
- cmdp->id[0], cmdp->id[1], cmdp->id[2]);
+  result = INTERNAL_SETXID_SYSCALL_NCS (cmdp);
   int error = 0;
   if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result)))
     {
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index 95c60a524a..80771e7788 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -39,6 +39,7 @@
 #include <libc-pointer-arith.h>
 #include <pthread-pids.h>
 #include <pthread_mutex_conf.h>
+#include <setxid-internal.h>
 
 #ifndef TLS_MULTIPLE_THREADS_IN_TCB
 /* Pointer to the corresponding variable in libc.  */
@@ -188,8 +189,7 @@ sighandler_setxid (int sig, siginfo_t *si, void *ctx)
       || si->si_code != SI_TKILL)
     return;
 
-  result = INTERNAL_SYSCALL_NCS (__xidcmd->syscall_no, 3, __xidcmd->id[0],
- __xidcmd->id[1], __xidcmd->id[2]);
+  result = INTERNAL_SETXID_SYSCALL_NCS (__xidcmd);
   int error = 0;
   if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result)))
     error = INTERNAL_SYSCALL_ERRNO (result);
diff --git a/sysdeps/nptl/setxid-internal.h b/sysdeps/nptl/setxid-internal.h
new file mode 100644
index 0000000000..d378b90db1
--- /dev/null
+++ b/sysdeps/nptl/setxid-internal.h
@@ -0,0 +1,21 @@
+/* INTERNAL_SETXID_SYSCALL_NCS.  Generic version.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#define INTERNAL_SETXID_SYSCALL_NCS(cmd) \
+  INTERNAL_SYSCALL_NCS (cmd->syscall_no, 3, cmd->id[0], cmd->id[1], \
+ cmd->id[2])
diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
index 16b768d8ba..1a6c984f96 100644
--- a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
+++ b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
@@ -5,6 +5,10 @@ ifeq ($(subdir),misc)
 sysdep_routines += arch_prctl
 endif
 
+ifeq ($(subdir),nptl)
+xtests += tst-setgroups
+endif
+
 ifeq ($(subdir),conform)
 # For bugs 16437 and 21279.
 conformtest-xfail-conds += x86_64-x32-linux
diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/setxid-internal.h b/sysdeps/unix/sysv/linux/x86_64/x32/setxid-internal.h
new file mode 100644
index 0000000000..bed30ba040
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86_64/x32/setxid-internal.h
@@ -0,0 +1,36 @@
+/* INTERNAL_SETXID_SYSCALL_NCS.  X32 version.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+/* Enforce zero-extension for the pointer argument in
+
+   int setgroups(size_t size, const gid_t *list);
+
+ */
+#define INTERNAL_SETXID_SYSCALL_NCS(cmd) \
+  ({ \
+    int __result; \
+    if (__glibc_unlikely (cmd->syscall_no == __NR_setgroups)) \
+      __result = INTERNAL_SYSCALL_NCS (__NR_setgroups, 2, \
+       cmd->id[0], \
+       (void *) cmd->id[1]); \
+    else \
+      __result = INTERNAL_SYSCALL_NCS (cmd->syscall_no, 3, \
+       cmd->id[0], cmd->id[1], \
+       cmd->id[2]); \
+    __result; \
+  })
diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
new file mode 100644
index 0000000000..a7167b0e26
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
@@ -0,0 +1,62 @@
+/* Basic test for setgroups
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+#include <limits.h>
+#include <grp.h>
+#include <errno.h>
+#include <error.h>
+#include <support/xthread.h>
+#include <support/support.h>
+#include <support/test-driver.h>
+#include <support/xunistd.h>
+
+static void *
+start_routine (void *args)
+{
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  int size;
+  /* NB: Stack address is at 0xfffXXXXX.  */
+  gid_t list[NGROUPS_MAX];
+  int status = EXIT_SUCCESS;
+
+  pthread_t thread = xpthread_create (NULL, start_routine, NULL);
+
+  size = getgroups (sizeof (list) / sizeof (list[0]), list);
+  if (size < 0)
+    {
+      status = EXIT_FAILURE;
+      error (0, errno, "getgroups failed");
+    }
+  if (setgroups (size, list) < 0 && errno != EPERM)
+    {
+      status = EXIT_FAILURE;
+      error (0, errno, "setgroups failed");
+    }
+
+  xpthread_join (thread);
+
+  return status;
+}
+
+#include <support/test-driver.c>
--
2.26.2

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Properly inline setgroups syscall [BZ #26248]

Sourceware - libc-alpha mailing list
* H. J. Lu via Libc-alpha:

> nptl has
>
> /* Opcodes and data types for communication with the signal handler to
>    change user/group IDs.  */
> struct xid_command
> {
>   int syscall_no;
>   long int id[3];
>   volatile int cntr;
>   volatile int error;
> };
>
>  /* This must be last, otherwise the current thread might not have
>      permissions to send SIGSETXID syscall to the other threads.  */
>   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
>                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
>
> But the second argument of setgroups syscal is a pointer:
>
>        int setgroups(size_t size, const gid_t *list);
>
> But on x32, pointers passed to syscall must have pointer type so that they
> will be zero-extended.
>
> Add <setxid-internal.h> to define INTERNAL_SETXID_SYSCALL_NCS and use it,
> instead of INTERNAL_SYSCALL_NCS, for SETXID syscalls.  X32 override it
> with pointer type for setgroups.  A testcase is added and setgroups
> returned with EFAULT when running as root without the fix.

Isn't it sufficient to change the type of id to unsigned long int[3]?
The UID arguments are unsigned on the kernel side, so no sign extension
is required.

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

[PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
On Thu, Jul 16, 2020 at 5:03 AM Florian Weimer <[hidden email]> wrote:

>
> * H. J. Lu via Libc-alpha:
>
> > nptl has
> >
> > /* Opcodes and data types for communication with the signal handler to
> >    change user/group IDs.  */
> > struct xid_command
> > {
> >   int syscall_no;
> >   long int id[3];
> >   volatile int cntr;
> >   volatile int error;
> > };
> >
> >  /* This must be last, otherwise the current thread might not have
> >      permissions to send SIGSETXID syscall to the other threads.  */
> >   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> >                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> >
> > But the second argument of setgroups syscal is a pointer:
> >
> >        int setgroups(size_t size, const gid_t *list);
> >
> > But on x32, pointers passed to syscall must have pointer type so that they
> > will be zero-extended.
> >
> > Add <setxid-internal.h> to define INTERNAL_SETXID_SYSCALL_NCS and use it,
> > instead of INTERNAL_SYSCALL_NCS, for SETXID syscalls.  X32 override it
> > with pointer type for setgroups.  A testcase is added and setgroups
> > returned with EFAULT when running as root without the fix.
>
> Isn't it sufficient to change the type of id to unsigned long int[3]?
> The UID arguments are unsigned on the kernel side, so no sign extension
> is required.
>
It works.  Here is the updated patch.  OK for master?

Thanks.

--
H.J.

0001-nptl-Zero-extend-arguments-to-SETXID-syscalls-BZ-262.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
* H. J. Lu via Libc-alpha:

>> Isn't it sufficient to change the type of id to unsigned long int[3]?
>> The UID arguments are unsigned on the kernel side, so no sign extension
>> is required.
>
> It works.  Here is the updated patch.  OK for master?

Does the test work if the list of supplementary groups is empty?

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
On Thu, Jul 16, 2020 at 8:57 AM Florian Weimer <[hidden email]> wrote:

>
> * H. J. Lu via Libc-alpha:
>
> >> Isn't it sufficient to change the type of id to unsigned long int[3]?
> >> The UID arguments are unsigned on the kernel side, so no sign extension
> >> is required.
> >
> > It works.  Here is the updated patch.  OK for master?
>
> Does the test work if the list of supplementary groups is empty?
>

My patch turns:

setgroups(0, bad address);

into

setgroups(0, good address);

It should be OK.

--
H.J.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Aurelien Jarno
In reply to this post by Sourceware - libc-alpha mailing list
On 2020-07-16 17:57, Florian Weimer via Libc-alpha wrote:
> * H. J. Lu via Libc-alpha:
>
> >> Isn't it sufficient to change the type of id to unsigned long int[3]?
> >> The UID arguments are unsigned on the kernel side, so no sign extension
> >> is required.
> >
> > It works.  Here is the updated patch.  OK for master?
>
> Does the test work if the list of supplementary groups is empty?

It depends how you defined "work". It doesn't fail wrongly when it
happens, so there is no risk of a failed test on other architectures.
OTOH it doesn't catch the issue on x32 as setgroups(0, bad_pointer) just
return successfully.

Aurelien

--
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[hidden email]                 http://www.aurel32.net
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Aurelien Jarno
In reply to this post by Sourceware - libc-alpha mailing list
On 2020-07-16 05:46, H.J. Lu via Libc-alpha wrote:

> On Thu, Jul 16, 2020 at 5:03 AM Florian Weimer <[hidden email]> wrote:
> >
> > * H. J. Lu via Libc-alpha:
> >
> > > nptl has
> > >
> > > /* Opcodes and data types for communication with the signal handler to
> > >    change user/group IDs.  */
> > > struct xid_command
> > > {
> > >   int syscall_no;
> > >   long int id[3];
> > >   volatile int cntr;
> > >   volatile int error;
> > > };
> > >
> > >  /* This must be last, otherwise the current thread might not have
> > >      permissions to send SIGSETXID syscall to the other threads.  */
> > >   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> > >                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> > >
> > > But the second argument of setgroups syscal is a pointer:
> > >
> > >        int setgroups(size_t size, const gid_t *list);
> > >
> > > But on x32, pointers passed to syscall must have pointer type so that they
> > > will be zero-extended.
> > >
> > > Add <setxid-internal.h> to define INTERNAL_SETXID_SYSCALL_NCS and use it,
> > > instead of INTERNAL_SYSCALL_NCS, for SETXID syscalls.  X32 override it
> > > with pointer type for setgroups.  A testcase is added and setgroups
> > > returned with EFAULT when running as root without the fix.
> >
> > Isn't it sufficient to change the type of id to unsigned long int[3]?
> > The UID arguments are unsigned on the kernel side, so no sign extension
> > is required.
> >
>
> It works.  Here is the updated patch.  OK for master?
>
> Thanks.
>
> --
> H.J.

> From 2af9e56c2306dc9d80a4476fa5b154a26a935557 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <[hidden email]>
> Date: Thu, 16 Jul 2020 03:37:10 -0700
> Subject: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]
>
> nptl has
>
> /* Opcodes and data types for communication with the signal handler to
>    change user/group IDs.  */
> struct xid_command
> {
>   int syscall_no;
>   long int id[3];
>   volatile int cntr;
>   volatile int error;
> };
>
>  /* This must be last, otherwise the current thread might not have
>      permissions to send SIGSETXID syscall to the other threads.  */
>   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
>                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
>
> But the second argument of setgroups syscal is a pointer:
>
>        int setgroups(size_t size, const gid_t *list);
>
> But on x32, pointers passed to syscall must have pointer type so that they
> will be zero-extended.  Since the XID arguments are unsigned on the kernel
> side, so no sign extension is required.  Change xid_command to
>
> struct xid_command
> {
>   int syscall_no;
>   unsigned long int id[3];
>   volatile int cntr;
>   volatile int error;
> };
>
> so that all arguments zero-extended.  A testcase is added for x32 and
> setgroups returned with EFAULT when running as root without the fix.
> ---
>  nptl/descr.h                                  |  8 ++-
>  sysdeps/unix/sysv/linux/x86_64/x32/Makefile   |  4 ++
>  .../sysv/linux/x86_64/x32/tst-setgroups.c     | 67 +++++++++++++++++++
>  3 files changed, 78 insertions(+), 1 deletion(-)
>  create mode 100644 sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
>
> diff --git a/nptl/descr.h b/nptl/descr.h
> index 6a509b6725..e98fe4084d 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -95,7 +95,13 @@ struct pthread_unwind_buf
>  struct xid_command
>  {
>    int syscall_no;
> -  long int id[3];
> +  /* Enforce zero-extension for the pointer argument in
> +
> +     int setgroups(size_t size, const gid_t *list);
> +
> +     Since the XID arguments are unsigned on the kernel side, so no sign
> +     extension is required.  */
> +  unsigned long int id[3];
>    volatile int cntr;
>    volatile int error; /* -1: no call yet, 0: success seen, >0: error seen.  */
>  };
> diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> index 16b768d8ba..1a6c984f96 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> @@ -5,6 +5,10 @@ ifeq ($(subdir),misc)
>  sysdep_routines += arch_prctl
>  endif
>  
> +ifeq ($(subdir),nptl)
> +xtests += tst-setgroups
> +endif
> +
>  ifeq ($(subdir),conform)
>  # For bugs 16437 and 21279.
>  conformtest-xfail-conds += x86_64-x32-linux
> diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> new file mode 100644
> index 0000000000..9895443278
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> @@ -0,0 +1,67 @@
> +/* Basic test for setgroups
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <grp.h>
> +#include <errno.h>
> +#include <error.h>
> +#include <support/xthread.h>
> +#include <support/support.h>
> +#include <support/test-driver.h>
> +#include <support/xunistd.h>
> +
> +static void *
> +start_routine (void *args)
> +{
> +  return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int size;
> +  /* NB: Stack address is at 0xfffXXXXX.  */
> +  gid_t list[NGROUPS_MAX];
> +  int status = EXIT_SUCCESS;
> +
> +  pthread_t thread = xpthread_create (NULL, start_routine, NULL);
> +
> +  size = getgroups (sizeof (list) / sizeof (list[0]), list);
> +  if (size < 0)
> +    {
> +      status = EXIT_FAILURE;
> +      error (0, errno, "getgroups failed");
> +    }
> +  if (setgroups (size, list) < 0)
> +    {

I guess the idea of using getgroups and setgroups comes from my initial
reproducer in the bug report. But you can actually do simpler by
skipping the getgroups and calling setgroups with a fixed single group.
It correctly handles the case where the list of supplementary groups
returned by getgroups is empty.

Aurelien

--
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[hidden email]                 http://www.aurel32.net
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
On Thu, Jul 16, 2020 at 12:45 PM Aurelien Jarno <[hidden email]> wrote:

>
> On 2020-07-16 05:46, H.J. Lu via Libc-alpha wrote:
> > On Thu, Jul 16, 2020 at 5:03 AM Florian Weimer <[hidden email]> wrote:
> > >
> > > * H. J. Lu via Libc-alpha:
> > >
> > > > nptl has
> > > >
> > > > /* Opcodes and data types for communication with the signal handler to
> > > >    change user/group IDs.  */
> > > > struct xid_command
> > > > {
> > > >   int syscall_no;
> > > >   long int id[3];
> > > >   volatile int cntr;
> > > >   volatile int error;
> > > > };
> > > >
> > > >  /* This must be last, otherwise the current thread might not have
> > > >      permissions to send SIGSETXID syscall to the other threads.  */
> > > >   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> > > >                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> > > >
> > > > But the second argument of setgroups syscal is a pointer:
> > > >
> > > >        int setgroups(size_t size, const gid_t *list);
> > > >
> > > > But on x32, pointers passed to syscall must have pointer type so that they
> > > > will be zero-extended.
> > > >
> > > > Add <setxid-internal.h> to define INTERNAL_SETXID_SYSCALL_NCS and use it,
> > > > instead of INTERNAL_SYSCALL_NCS, for SETXID syscalls.  X32 override it
> > > > with pointer type for setgroups.  A testcase is added and setgroups
> > > > returned with EFAULT when running as root without the fix.
> > >
> > > Isn't it sufficient to change the type of id to unsigned long int[3]?
> > > The UID arguments are unsigned on the kernel side, so no sign extension
> > > is required.
> > >
> >
> > It works.  Here is the updated patch.  OK for master?
> >
> > Thanks.
> >
> > --
> > H.J.
>
> > From 2af9e56c2306dc9d80a4476fa5b154a26a935557 Mon Sep 17 00:00:00 2001
> > From: "H.J. Lu" <[hidden email]>
> > Date: Thu, 16 Jul 2020 03:37:10 -0700
> > Subject: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]
> >
> > nptl has
> >
> > /* Opcodes and data types for communication with the signal handler to
> >    change user/group IDs.  */
> > struct xid_command
> > {
> >   int syscall_no;
> >   long int id[3];
> >   volatile int cntr;
> >   volatile int error;
> > };
> >
> >  /* This must be last, otherwise the current thread might not have
> >      permissions to send SIGSETXID syscall to the other threads.  */
> >   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> >                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> >
> > But the second argument of setgroups syscal is a pointer:
> >
> >        int setgroups(size_t size, const gid_t *list);
> >
> > But on x32, pointers passed to syscall must have pointer type so that they
> > will be zero-extended.  Since the XID arguments are unsigned on the kernel
> > side, so no sign extension is required.  Change xid_command to
> >
> > struct xid_command
> > {
> >   int syscall_no;
> >   unsigned long int id[3];
> >   volatile int cntr;
> >   volatile int error;
> > };
> >
> > so that all arguments zero-extended.  A testcase is added for x32 and
> > setgroups returned with EFAULT when running as root without the fix.
> > ---
> >  nptl/descr.h                                  |  8 ++-
> >  sysdeps/unix/sysv/linux/x86_64/x32/Makefile   |  4 ++
> >  .../sysv/linux/x86_64/x32/tst-setgroups.c     | 67 +++++++++++++++++++
> >  3 files changed, 78 insertions(+), 1 deletion(-)
> >  create mode 100644 sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> >
> > diff --git a/nptl/descr.h b/nptl/descr.h
> > index 6a509b6725..e98fe4084d 100644
> > --- a/nptl/descr.h
> > +++ b/nptl/descr.h
> > @@ -95,7 +95,13 @@ struct pthread_unwind_buf
> >  struct xid_command
> >  {
> >    int syscall_no;
> > -  long int id[3];
> > +  /* Enforce zero-extension for the pointer argument in
> > +
> > +     int setgroups(size_t size, const gid_t *list);
> > +
> > +     Since the XID arguments are unsigned on the kernel side, so no sign
> > +     extension is required.  */
> > +  unsigned long int id[3];
> >    volatile int cntr;
> >    volatile int error; /* -1: no call yet, 0: success seen, >0: error seen.  */
> >  };
> > diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> > index 16b768d8ba..1a6c984f96 100644
> > --- a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> > +++ b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> > @@ -5,6 +5,10 @@ ifeq ($(subdir),misc)
> >  sysdep_routines += arch_prctl
> >  endif
> >
> > +ifeq ($(subdir),nptl)
> > +xtests += tst-setgroups
> > +endif
> > +
> >  ifeq ($(subdir),conform)
> >  # For bugs 16437 and 21279.
> >  conformtest-xfail-conds += x86_64-x32-linux
> > diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> > new file mode 100644
> > index 0000000000..9895443278
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> > @@ -0,0 +1,67 @@
> > +/* Basic test for setgroups
> > +   Copyright (C) 2020 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +#include <stdlib.h>
> > +#include <limits.h>
> > +#include <grp.h>
> > +#include <errno.h>
> > +#include <error.h>
> > +#include <support/xthread.h>
> > +#include <support/support.h>
> > +#include <support/test-driver.h>
> > +#include <support/xunistd.h>
> > +
> > +static void *
> > +start_routine (void *args)
> > +{
> > +  return NULL;
> > +}
> > +
> > +static int
> > +do_test (void)
> > +{
> > +  int size;
> > +  /* NB: Stack address is at 0xfffXXXXX.  */
> > +  gid_t list[NGROUPS_MAX];
> > +  int status = EXIT_SUCCESS;
> > +
> > +  pthread_t thread = xpthread_create (NULL, start_routine, NULL);
> > +
> > +  size = getgroups (sizeof (list) / sizeof (list[0]), list);
> > +  if (size < 0)
> > +    {
> > +      status = EXIT_FAILURE;
> > +      error (0, errno, "getgroups failed");
> > +    }
> > +  if (setgroups (size, list) < 0)
> > +    {
>
> I guess the idea of using getgroups and setgroups comes from my initial
> reproducer in the bug report. But you can actually do simpler by
> skipping the getgroups and calling setgroups with a fixed single group.
> It correctly handles the case where the list of supplementary groups
> returned by getgroups is empty.
>

This test is simple enough.

Carlos, I'd like to get it fixed in 2.32.

Thanks.

--
H.J.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
On 7/16/20 5:42 PM, H.J. Lu wrote:

> On Thu, Jul 16, 2020 at 12:45 PM Aurelien Jarno <[hidden email]> wrote:
>>
>> On 2020-07-16 05:46, H.J. Lu via Libc-alpha wrote:
>>> On Thu, Jul 16, 2020 at 5:03 AM Florian Weimer <[hidden email]> wrote:
>>>>
>>>> * H. J. Lu via Libc-alpha:
>>>>
>>>>> nptl has
>>>>>
>>>>> /* Opcodes and data types for communication with the signal handler to
>>>>>    change user/group IDs.  */
>>>>> struct xid_command
>>>>> {
>>>>>   int syscall_no;
>>>>>   long int id[3];
>>>>>   volatile int cntr;
>>>>>   volatile int error;
>>>>> };
>>>>>
>>>>>  /* This must be last, otherwise the current thread might not have
>>>>>      permissions to send SIGSETXID syscall to the other threads.  */
>>>>>   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
>>>>>                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
>>>>>
>>>>> But the second argument of setgroups syscal is a pointer:
>>>>>
>>>>>        int setgroups(size_t size, const gid_t *list);
>>>>>
>>>>> But on x32, pointers passed to syscall must have pointer type so that they
>>>>> will be zero-extended.
>>>>>
>>>>> Add <setxid-internal.h> to define INTERNAL_SETXID_SYSCALL_NCS and use it,
>>>>> instead of INTERNAL_SYSCALL_NCS, for SETXID syscalls.  X32 override it
>>>>> with pointer type for setgroups.  A testcase is added and setgroups
>>>>> returned with EFAULT when running as root without the fix.
>>>>
>>>> Isn't it sufficient to change the type of id to unsigned long int[3]?
>>>> The UID arguments are unsigned on the kernel side, so no sign extension
>>>> is required.
>>>>
>>>
>>> It works.  Here is the updated patch.  OK for master?
>>>
>>> Thanks.
>>>
>>> --
>>> H.J.
>>
>>> From 2af9e56c2306dc9d80a4476fa5b154a26a935557 Mon Sep 17 00:00:00 2001
>>> From: "H.J. Lu" <[hidden email]>
>>> Date: Thu, 16 Jul 2020 03:37:10 -0700
>>> Subject: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]
>>>
>>> nptl has
>>>
>>> /* Opcodes and data types for communication with the signal handler to
>>>    change user/group IDs.  */
>>> struct xid_command
>>> {
>>>   int syscall_no;
>>>   long int id[3];
>>>   volatile int cntr;
>>>   volatile int error;
>>> };
>>>
>>>  /* This must be last, otherwise the current thread might not have
>>>      permissions to send SIGSETXID syscall to the other threads.  */
>>>   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
>>>                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
>>>
>>> But the second argument of setgroups syscal is a pointer:
>>>
>>>        int setgroups(size_t size, const gid_t *list);
>>>
>>> But on x32, pointers passed to syscall must have pointer type so that they
>>> will be zero-extended.  Since the XID arguments are unsigned on the kernel
>>> side, so no sign extension is required.  Change xid_command to
>>>
>>> struct xid_command
>>> {
>>>   int syscall_no;
>>>   unsigned long int id[3];
>>>   volatile int cntr;
>>>   volatile int error;
>>> };
>>>
>>> so that all arguments zero-extended.  A testcase is added for x32 and
>>> setgroups returned with EFAULT when running as root without the fix.
>>> ---
>>>  nptl/descr.h                                  |  8 ++-
>>>  sysdeps/unix/sysv/linux/x86_64/x32/Makefile   |  4 ++
>>>  .../sysv/linux/x86_64/x32/tst-setgroups.c     | 67 +++++++++++++++++++
>>>  3 files changed, 78 insertions(+), 1 deletion(-)
>>>  create mode 100644 sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
>>>
>>> diff --git a/nptl/descr.h b/nptl/descr.h
>>> index 6a509b6725..e98fe4084d 100644
>>> --- a/nptl/descr.h
>>> +++ b/nptl/descr.h
>>> @@ -95,7 +95,13 @@ struct pthread_unwind_buf
>>>  struct xid_command
>>>  {
>>>    int syscall_no;
>>> -  long int id[3];
>>> +  /* Enforce zero-extension for the pointer argument in
>>> +
>>> +     int setgroups(size_t size, const gid_t *list);
>>> +
>>> +     Since the XID arguments are unsigned on the kernel side, so no sign
>>> +     extension is required.  */
>>> +  unsigned long int id[3];
>>>    volatile int cntr;
>>>    volatile int error; /* -1: no call yet, 0: success seen, >0: error seen.  */
>>>  };
>>> diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
>>> index 16b768d8ba..1a6c984f96 100644
>>> --- a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
>>> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
>>> @@ -5,6 +5,10 @@ ifeq ($(subdir),misc)
>>>  sysdep_routines += arch_prctl
>>>  endif
>>>
>>> +ifeq ($(subdir),nptl)
>>> +xtests += tst-setgroups
>>> +endif
>>> +
>>>  ifeq ($(subdir),conform)
>>>  # For bugs 16437 and 21279.
>>>  conformtest-xfail-conds += x86_64-x32-linux
>>> diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
>>> new file mode 100644
>>> index 0000000000..9895443278
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
>>> @@ -0,0 +1,67 @@
>>> +/* Basic test for setgroups
>>> +   Copyright (C) 2020 Free Software Foundation, Inc.
>>> +   This file is part of the GNU C Library.
>>> +
>>> +   The GNU C Library is free software; you can redistribute it and/or
>>> +   modify it under the terms of the GNU Lesser General Public
>>> +   License as published by the Free Software Foundation; either
>>> +   version 2.1 of the License, or (at your option) any later version.
>>> +
>>> +   The GNU C Library is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> +   Lesser General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU Lesser General Public
>>> +   License along with the GNU C Library; if not, see
>>> +   <https://www.gnu.org/licenses/>.  */
>>> +
>>> +#include <stdlib.h>
>>> +#include <limits.h>
>>> +#include <grp.h>
>>> +#include <errno.h>
>>> +#include <error.h>
>>> +#include <support/xthread.h>
>>> +#include <support/support.h>
>>> +#include <support/test-driver.h>
>>> +#include <support/xunistd.h>
>>> +
>>> +static void *
>>> +start_routine (void *args)
>>> +{
>>> +  return NULL;
>>> +}
>>> +
>>> +static int
>>> +do_test (void)
>>> +{
>>> +  int size;
>>> +  /* NB: Stack address is at 0xfffXXXXX.  */
>>> +  gid_t list[NGROUPS_MAX];
>>> +  int status = EXIT_SUCCESS;
>>> +
>>> +  pthread_t thread = xpthread_create (NULL, start_routine, NULL);
>>> +
>>> +  size = getgroups (sizeof (list) / sizeof (list[0]), list);
>>> +  if (size < 0)
>>> +    {
>>> +      status = EXIT_FAILURE;
>>> +      error (0, errno, "getgroups failed");
>>> +    }
>>> +  if (setgroups (size, list) < 0)
>>> +    {
>>
>> I guess the idea of using getgroups and setgroups comes from my initial
>> reproducer in the bug report. But you can actually do simpler by
>> skipping the getgroups and calling setgroups with a fixed single group.
>> It correctly handles the case where the list of supplementary groups
>> returned by getgroups is empty.
>>
>
> This test is simple enough.
>
> Carlos, I'd like to get it fixed in 2.32.

Please point me at the final version you want reviewed and I'll do that
first thing tomorrow morning.

--
Cheers,
Carlos.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
On Thu, Jul 16, 2020 at 7:14 PM Carlos O'Donell <[hidden email]> wrote:

>
> On 7/16/20 5:42 PM, H.J. Lu wrote:
> > On Thu, Jul 16, 2020 at 12:45 PM Aurelien Jarno <[hidden email]> wrote:
> >>
> >> On 2020-07-16 05:46, H.J. Lu via Libc-alpha wrote:
> >>> On Thu, Jul 16, 2020 at 5:03 AM Florian Weimer <[hidden email]> wrote:
> >>>>
> >>>> * H. J. Lu via Libc-alpha:
> >>>>
> >>>>> nptl has
> >>>>>
> >>>>> /* Opcodes and data types for communication with the signal handler to
> >>>>>    change user/group IDs.  */
> >>>>> struct xid_command
> >>>>> {
> >>>>>   int syscall_no;
> >>>>>   long int id[3];
> >>>>>   volatile int cntr;
> >>>>>   volatile int error;
> >>>>> };
> >>>>>
> >>>>>  /* This must be last, otherwise the current thread might not have
> >>>>>      permissions to send SIGSETXID syscall to the other threads.  */
> >>>>>   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> >>>>>                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> >>>>>
> >>>>> But the second argument of setgroups syscal is a pointer:
> >>>>>
> >>>>>        int setgroups(size_t size, const gid_t *list);
> >>>>>
> >>>>> But on x32, pointers passed to syscall must have pointer type so that they
> >>>>> will be zero-extended.
> >>>>>
> >>>>> Add <setxid-internal.h> to define INTERNAL_SETXID_SYSCALL_NCS and use it,
> >>>>> instead of INTERNAL_SYSCALL_NCS, for SETXID syscalls.  X32 override it
> >>>>> with pointer type for setgroups.  A testcase is added and setgroups
> >>>>> returned with EFAULT when running as root without the fix.
> >>>>
> >>>> Isn't it sufficient to change the type of id to unsigned long int[3]?
> >>>> The UID arguments are unsigned on the kernel side, so no sign extension
> >>>> is required.
> >>>>
> >>>
> >>> It works.  Here is the updated patch.  OK for master?
> >>>
> >>> Thanks.
> >>>
> >>> --
> >>> H.J.
> >>
> >>> From 2af9e56c2306dc9d80a4476fa5b154a26a935557 Mon Sep 17 00:00:00 2001
> >>> From: "H.J. Lu" <[hidden email]>
> >>> Date: Thu, 16 Jul 2020 03:37:10 -0700
> >>> Subject: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]
> >>>
> >>> nptl has
> >>>
> >>> /* Opcodes and data types for communication with the signal handler to
> >>>    change user/group IDs.  */
> >>> struct xid_command
> >>> {
> >>>   int syscall_no;
> >>>   long int id[3];
> >>>   volatile int cntr;
> >>>   volatile int error;
> >>> };
> >>>
> >>>  /* This must be last, otherwise the current thread might not have
> >>>      permissions to send SIGSETXID syscall to the other threads.  */
> >>>   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> >>>                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> >>>
> >>> But the second argument of setgroups syscal is a pointer:
> >>>
> >>>        int setgroups(size_t size, const gid_t *list);
> >>>
> >>> But on x32, pointers passed to syscall must have pointer type so that they
> >>> will be zero-extended.  Since the XID arguments are unsigned on the kernel
> >>> side, so no sign extension is required.  Change xid_command to
> >>>
> >>> struct xid_command
> >>> {
> >>>   int syscall_no;
> >>>   unsigned long int id[3];
> >>>   volatile int cntr;
> >>>   volatile int error;
> >>> };
> >>>
> >>> so that all arguments zero-extended.  A testcase is added for x32 and
> >>> setgroups returned with EFAULT when running as root without the fix.
> >>> ---
> >>>  nptl/descr.h                                  |  8 ++-
> >>>  sysdeps/unix/sysv/linux/x86_64/x32/Makefile   |  4 ++
> >>>  .../sysv/linux/x86_64/x32/tst-setgroups.c     | 67 +++++++++++++++++++
> >>>  3 files changed, 78 insertions(+), 1 deletion(-)
> >>>  create mode 100644 sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> >>>
> >>> diff --git a/nptl/descr.h b/nptl/descr.h
> >>> index 6a509b6725..e98fe4084d 100644
> >>> --- a/nptl/descr.h
> >>> +++ b/nptl/descr.h
> >>> @@ -95,7 +95,13 @@ struct pthread_unwind_buf
> >>>  struct xid_command
> >>>  {
> >>>    int syscall_no;
> >>> -  long int id[3];
> >>> +  /* Enforce zero-extension for the pointer argument in
> >>> +
> >>> +     int setgroups(size_t size, const gid_t *list);
> >>> +
> >>> +     Since the XID arguments are unsigned on the kernel side, so no sign
> >>> +     extension is required.  */
> >>> +  unsigned long int id[3];
> >>>    volatile int cntr;
> >>>    volatile int error; /* -1: no call yet, 0: success seen, >0: error seen.  */
> >>>  };
> >>> diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> >>> index 16b768d8ba..1a6c984f96 100644
> >>> --- a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> >>> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> >>> @@ -5,6 +5,10 @@ ifeq ($(subdir),misc)
> >>>  sysdep_routines += arch_prctl
> >>>  endif
> >>>
> >>> +ifeq ($(subdir),nptl)
> >>> +xtests += tst-setgroups
> >>> +endif
> >>> +
> >>>  ifeq ($(subdir),conform)
> >>>  # For bugs 16437 and 21279.
> >>>  conformtest-xfail-conds += x86_64-x32-linux
> >>> diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> >>> new file mode 100644
> >>> index 0000000000..9895443278
> >>> --- /dev/null
> >>> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> >>> @@ -0,0 +1,67 @@
> >>> +/* Basic test for setgroups
> >>> +   Copyright (C) 2020 Free Software Foundation, Inc.
> >>> +   This file is part of the GNU C Library.
> >>> +
> >>> +   The GNU C Library is free software; you can redistribute it and/or
> >>> +   modify it under the terms of the GNU Lesser General Public
> >>> +   License as published by the Free Software Foundation; either
> >>> +   version 2.1 of the License, or (at your option) any later version.
> >>> +
> >>> +   The GNU C Library is distributed in the hope that it will be useful,
> >>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >>> +   Lesser General Public License for more details.
> >>> +
> >>> +   You should have received a copy of the GNU Lesser General Public
> >>> +   License along with the GNU C Library; if not, see
> >>> +   <https://www.gnu.org/licenses/>.  */
> >>> +
> >>> +#include <stdlib.h>
> >>> +#include <limits.h>
> >>> +#include <grp.h>
> >>> +#include <errno.h>
> >>> +#include <error.h>
> >>> +#include <support/xthread.h>
> >>> +#include <support/support.h>
> >>> +#include <support/test-driver.h>
> >>> +#include <support/xunistd.h>
> >>> +
> >>> +static void *
> >>> +start_routine (void *args)
> >>> +{
> >>> +  return NULL;
> >>> +}
> >>> +
> >>> +static int
> >>> +do_test (void)
> >>> +{
> >>> +  int size;
> >>> +  /* NB: Stack address is at 0xfffXXXXX.  */
> >>> +  gid_t list[NGROUPS_MAX];
> >>> +  int status = EXIT_SUCCESS;
> >>> +
> >>> +  pthread_t thread = xpthread_create (NULL, start_routine, NULL);
> >>> +
> >>> +  size = getgroups (sizeof (list) / sizeof (list[0]), list);
> >>> +  if (size < 0)
> >>> +    {
> >>> +      status = EXIT_FAILURE;
> >>> +      error (0, errno, "getgroups failed");
> >>> +    }
> >>> +  if (setgroups (size, list) < 0)
> >>> +    {
> >>
> >> I guess the idea of using getgroups and setgroups comes from my initial
> >> reproducer in the bug report. But you can actually do simpler by
> >> skipping the getgroups and calling setgroups with a fixed single group.
> >> It correctly handles the case where the list of supplementary groups
> >> returned by getgroups is empty.
> >>
> >
> > This test is simple enough.
> >
> > Carlos, I'd like to get it fixed in 2.32.
>
> Please point me at the final version you want reviewed and I'll do that
> first thing tomorrow morning.
>

Here is the final version:

https://sourceware.org/pipermail/libc-alpha/2020-July/116390.html

--
H.J.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
In reply to this post by Sourceware - libc-alpha mailing list
On 7/16/20 8:46 AM, H.J. Lu via Libc-alpha wrote:

> On Thu, Jul 16, 2020 at 5:03 AM Florian Weimer <[hidden email]> wrote:
>>
>> * H. J. Lu via Libc-alpha:
>>
>>> nptl has
>>>
>>> /* Opcodes and data types for communication with the signal handler to
>>>    change user/group IDs.  */
>>> struct xid_command
>>> {
>>>   int syscall_no;
>>>   long int id[3];
>>>   volatile int cntr;
>>>   volatile int error;
>>> };
>>>
>>>  /* This must be last, otherwise the current thread might not have
>>>      permissions to send SIGSETXID syscall to the other threads.  */
>>>   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
>>>                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
>>>
>>> But the second argument of setgroups syscal is a pointer:
>>>
>>>        int setgroups(size_t size, const gid_t *list);
>>>
>>> But on x32, pointers passed to syscall must have pointer type so that they
>>> will be zero-extended.
>>>
>>> Add <setxid-internal.h> to define INTERNAL_SETXID_SYSCALL_NCS and use it,
>>> instead of INTERNAL_SYSCALL_NCS, for SETXID syscalls.  X32 override it
>>> with pointer type for setgroups.  A testcase is added and setgroups
>>> returned with EFAULT when running as root without the fix.
>>
>> Isn't it sufficient to change the type of id to unsigned long int[3]?
>> The UID arguments are unsigned on the kernel side, so no sign extension
>> is required.
>>
>
> It works.  Here is the updated patch.  OK for master?
>
> Thanks.
>

This test should run in a container, and it should attempt two setgroups
calls, one with groups and one empty with a bad address.

> From 2af9e56c2306dc9d80a4476fa5b154a26a935557 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <[hidden email]>
> Date: Thu, 16 Jul 2020 03:37:10 -0700
> Subject: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]
>
> nptl has
>
> /* Opcodes and data types for communication with the signal handler to
>    change user/group IDs.  */
> struct xid_command
> {
>   int syscall_no;
>   long int id[3];
>   volatile int cntr;
>   volatile int error;
> };
>
>  /* This must be last, otherwise the current thread might not have
>      permissions to send SIGSETXID syscall to the other threads.  */
>   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
>                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
>
> But the second argument of setgroups syscal is a pointer:
>
>        int setgroups(size_t size, const gid_t *list);
>
> But on x32, pointers passed to syscall must have pointer type so that they
> will be zero-extended.  Since the XID arguments are unsigned on the kernel
> side, so no sign extension is required.  Change xid_command to
>
> struct xid_command
> {
>   int syscall_no;
>   unsigned long int id[3];
>   volatile int cntr;
>   volatile int error;
> };
>
> so that all arguments zero-extended.  A testcase is added for x32 and
> setgroups returned with EFAULT when running as root without the fix.
> ---
>  nptl/descr.h                                  |  8 ++-
>  sysdeps/unix/sysv/linux/x86_64/x32/Makefile   |  4 ++
>  .../sysv/linux/x86_64/x32/tst-setgroups.c     | 67 +++++++++++++++++++
>  3 files changed, 78 insertions(+), 1 deletion(-)
>  create mode 100644 sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
>
> diff --git a/nptl/descr.h b/nptl/descr.h
> index 6a509b6725..e98fe4084d 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -95,7 +95,13 @@ struct pthread_unwind_buf
>  struct xid_command
>  {
>    int syscall_no;
> -  long int id[3];
> +  /* Enforce zero-extension for the pointer argument in
> +
> +     int setgroups(size_t size, const gid_t *list);
> +

Suggest:

The kernel XID arguments are unsigned and do not require sign extension.

> +     Since the XID arguments are unsigned on the kernel side, so no sign
> +     extension is required.  */


> +  unsigned long int id[3];
>    volatile int cntr;
>    volatile int error; /* -1: no call yet, 0: success seen, >0: error seen.  */
>  };
> diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> index 16b768d8ba..1a6c984f96 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> @@ -5,6 +5,10 @@ ifeq ($(subdir),misc)
>  sysdep_routines += arch_prctl
>  endif
>  
> +ifeq ($(subdir),nptl)
> +xtests += tst-setgroups
> +endif

Use tests-container and use su to become root in the container.

> +
>  ifeq ($(subdir),conform)
>  # For bugs 16437 and 21279.
>  conformtest-xfail-conds += x86_64-x32-linux
> diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> new file mode 100644
> index 0000000000..9895443278
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> @@ -0,0 +1,67 @@
> +/* Basic test for setgroups

This is a specific test for this bug.

Suggest:

Test setgroups as root and in the presence of threads (Bug 26248)

> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <grp.h>
> +#include <errno.h>
> +#include <error.h>
> +#include <support/xthread.h>
> +#include <support/support.h>
> +#include <support/test-driver.h>
> +#include <support/xunistd.h>
> +

Suggest:

/* The purpose of this test is to test the setgroups API as root
   and in the presence of threads.  Once we create a thread the
   setgroups implementation must ensure that all threads are set
   to the same group and this operation should not fail. Lastly
   we test setgroups with a zero sized group and a bad address
   and verify we get EPERM.  */

> +static void *
> +start_routine (void *args)
> +{
> +  return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int size;
> +  /* NB: Stack address is at 0xfffXXXXX.  */
> +  gid_t list[NGROUPS_MAX];
> +  int status = EXIT_SUCCESS;
> +
> +  pthread_t thread = xpthread_create (NULL, start_routine, NULL);
> +
> +  size = getgroups (sizeof (list) / sizeof (list[0]), list);
> +  if (size < 0)
> +    {
> +      status = EXIT_FAILURE;
> +      error (0, errno, "getgroups failed");
> +    }
> +  if (setgroups (size, list) < 0)
> +    {
> +      if (errno == EPERM)
> + status = EXIT_UNSUPPORTED;
> +      else
> + {
> +  status = EXIT_FAILURE;
> +  error (0, errno, "setgroups failed");
> + }
> +    }


Test again with setgroups (0, bad addresss) ?

> +
> +  xpthread_join (thread);
> +
> +  return status;
> +}
> +
> +#include <support/test-driver.c>
> --
> 2.26.2
>


--
Cheers,
Carlos.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
* Carlos O'Donell:

> This test should run in a container, and it should attempt two setgroups
> calls, one with groups and one empty with a bad address.

Why do you think this needs a container?

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
On 7/17/20 11:13 AM, Florian Weimer wrote:
> * Carlos O'Donell:
>
>> This test should run in a container, and it should attempt two setgroups
>> calls, one with groups and one empty with a bad address.
>
> Why do you think this needs a container?

We are trying to successfully call setgroups(), and to do that we need
CAP_SETGID. The way this test is exercising this is by making the test
an xtests which can require root and thus you get CAP_SETGID in that way.

My suggestion is to move the test from xtests to tests-container to increase
the usage of the test. In the container we have a CLONE_NEWUSER so we have
a distinct usersnamespace that can be used in conjunction with becoming
root, getting CAP_SETGID, and calling setgroups() without restricting this
test to `make xcheck`.

I see that we don't explicitly say `make xcheck` may require root.
Is this something I just taught myself implicitly? :-)

Note: We may need to adjust the gid_map writing code in test-container.

--
Cheers,
Carlos.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
On Fri, Jul 17, 2020 at 8:52 AM Carlos O'Donell <[hidden email]> wrote:

>
> On 7/17/20 11:13 AM, Florian Weimer wrote:
> > * Carlos O'Donell:
> >
> >> This test should run in a container, and it should attempt two setgroups
> >> calls, one with groups and one empty with a bad address.
> >
> > Why do you think this needs a container?
>
> We are trying to successfully call setgroups(), and to do that we need
> CAP_SETGID. The way this test is exercising this is by making the test
> an xtests which can require root and thus you get CAP_SETGID in that way.
>
> My suggestion is to move the test from xtests to tests-container to increase
> the usage of the test. In the container we have a CLONE_NEWUSER so we have
> a distinct usersnamespace that can be used in conjunction with becoming
> root, getting CAP_SETGID, and calling setgroups() without restricting this
> test to `make xcheck`.

I see su in tst-localedef-path-norm.script.  But when I removed "su" from
tst-localedef-path-norm.script, tst-localedef-path-norm still passed.  There are

[hjl@gnu-cfl-2 build-x86_64-linux]$ ls -l testroot.root
total 44
drwxr-xr-x 2 hjl hjl 4096 Jul 17 09:16 bin
drwxr-xr-x 2 hjl hjl 4096 Jul 17 12:23 dev
drwxr-xr-x 2 hjl hjl 4096 Jul 17 09:34 etc
drwxr-xr-x 4 hjl hjl 4096 Jul 17 12:23 export
-rw-r--r-- 1 hjl hjl    0 Jul 17 09:16 install.stamp
drwxr-xr-x 2 hjl hjl 4096 Jul 17 09:16 libx32
-rw-r--r-- 1 hjl hjl    0 Jul 17 12:26 lock.fd
drwxr-xr-x 5 hjl hjl 4096 Jul 17 12:23 output
drwxr-xr-x 2 hjl hjl 4096 Jul 17 12:23 proc
drwxr-xr-x 2 hjl hjl 4096 Jul 17 09:16 sbin
drwxr-xr-x 2 hjl hjl 4096 Jul 17 12:23 tmp
drwxr-xr-x 9 hjl hjl 4096 Jul 17 09:16 usr
drwxr-xr-x 3 hjl hjl 4096 Jul 17 09:34 var
[hjl@gnu-cfl-2 build-x86_64-linux]$

I don't think su is needed since testroot.root is owned by me.

> I see that we don't explicitly say `make xcheck` may require root.
> Is this something I just taught myself implicitly? :-)
>
> Note: We may need to adjust the gid_map writing code in test-container.
>

Can you help me to make tst-setgroups pass when not running as root?


--
H.J.
Reply | Threaded
Open this post in threaded view
|

V2 [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
In reply to this post by Sourceware - libc-alpha mailing list
On Fri, Jul 17, 2020 at 8:02 AM Carlos O'Donell <[hidden email]> wrote:

>
> On 7/16/20 8:46 AM, H.J. Lu via Libc-alpha wrote:
> > On Thu, Jul 16, 2020 at 5:03 AM Florian Weimer <[hidden email]> wrote:
> >>
> >> * H. J. Lu via Libc-alpha:
> >>
> >>> nptl has
> >>>
> >>> /* Opcodes and data types for communication with the signal handler to
> >>>    change user/group IDs.  */
> >>> struct xid_command
> >>> {
> >>>   int syscall_no;
> >>>   long int id[3];
> >>>   volatile int cntr;
> >>>   volatile int error;
> >>> };
> >>>
> >>>  /* This must be last, otherwise the current thread might not have
> >>>      permissions to send SIGSETXID syscall to the other threads.  */
> >>>   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> >>>                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> >>>
> >>> But the second argument of setgroups syscal is a pointer:
> >>>
> >>>        int setgroups(size_t size, const gid_t *list);
> >>>
> >>> But on x32, pointers passed to syscall must have pointer type so that they
> >>> will be zero-extended.
> >>>
> >>> Add <setxid-internal.h> to define INTERNAL_SETXID_SYSCALL_NCS and use it,
> >>> instead of INTERNAL_SYSCALL_NCS, for SETXID syscalls.  X32 override it
> >>> with pointer type for setgroups.  A testcase is added and setgroups
> >>> returned with EFAULT when running as root without the fix.
> >>
> >> Isn't it sufficient to change the type of id to unsigned long int[3]?
> >> The UID arguments are unsigned on the kernel side, so no sign extension
> >> is required.
> >>
> >
> > It works.  Here is the updated patch.  OK for master?
> >
> > Thanks.
> >
>
> This test should run in a container, and it should attempt two setgroups
> calls, one with groups and one empty with a bad address.
Fixed.

> > From 2af9e56c2306dc9d80a4476fa5b154a26a935557 Mon Sep 17 00:00:00 2001
> > From: "H.J. Lu" <[hidden email]>
> > Date: Thu, 16 Jul 2020 03:37:10 -0700
> > Subject: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]
> >
> > nptl has
> >
> > /* Opcodes and data types for communication with the signal handler to
> >    change user/group IDs.  */
> > struct xid_command
> > {
> >   int syscall_no;
> >   long int id[3];
> >   volatile int cntr;
> >   volatile int error;
> > };
> >
> >  /* This must be last, otherwise the current thread might not have
> >      permissions to send SIGSETXID syscall to the other threads.  */
> >   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> >                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> >
> > But the second argument of setgroups syscal is a pointer:
> >
> >        int setgroups(size_t size, const gid_t *list);
> >
> > But on x32, pointers passed to syscall must have pointer type so that they
> > will be zero-extended.  Since the XID arguments are unsigned on the kernel
> > side, so no sign extension is required.  Change xid_command to
> >
> > struct xid_command
> > {
> >   int syscall_no;
> >   unsigned long int id[3];
> >   volatile int cntr;
> >   volatile int error;
> > };
> >
> > so that all arguments zero-extended.  A testcase is added for x32 and
> > setgroups returned with EFAULT when running as root without the fix.
> > ---
> >  nptl/descr.h                                  |  8 ++-
> >  sysdeps/unix/sysv/linux/x86_64/x32/Makefile   |  4 ++
> >  .../sysv/linux/x86_64/x32/tst-setgroups.c     | 67 +++++++++++++++++++
> >  3 files changed, 78 insertions(+), 1 deletion(-)
> >  create mode 100644 sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> >
> > diff --git a/nptl/descr.h b/nptl/descr.h
> > index 6a509b6725..e98fe4084d 100644
> > --- a/nptl/descr.h
> > +++ b/nptl/descr.h
> > @@ -95,7 +95,13 @@ struct pthread_unwind_buf
> >  struct xid_command
> >  {
> >    int syscall_no;
> > -  long int id[3];
> > +  /* Enforce zero-extension for the pointer argument in
> > +
> > +     int setgroups(size_t size, const gid_t *list);
> > +
>
> Suggest:
>
> The kernel XID arguments are unsigned and do not require sign extension.
Fixed.

> > +     Since the XID arguments are unsigned on the kernel side, so no sign
> > +     extension is required.  */
>
>
> > +  unsigned long int id[3];
> >    volatile int cntr;
> >    volatile int error; /* -1: no call yet, 0: success seen, >0: error seen.  */
> >  };
> > diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> > index 16b768d8ba..1a6c984f96 100644
> > --- a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> > +++ b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
> > @@ -5,6 +5,10 @@ ifeq ($(subdir),misc)
> >  sysdep_routines += arch_prctl
> >  endif
> >
> > +ifeq ($(subdir),nptl)
> > +xtests += tst-setgroups
> > +endif
>
> Use tests-container and use su to become root in the container.
Since I don't know how to give random user CAP_SETGID privilege via
tests-container,  I leave it as xtests.

> > +
> >  ifeq ($(subdir),conform)
> >  # For bugs 16437 and 21279.
> >  conformtest-xfail-conds += x86_64-x32-linux
> > diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> > new file mode 100644
> > index 0000000000..9895443278
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> > @@ -0,0 +1,67 @@
> > +/* Basic test for setgroups
>
> This is a specific test for this bug.
>
> Suggest:
>
> Test setgroups as root and in the presence of threads (Bug 26248)
Fixed.

> > +   Copyright (C) 2020 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +#include <stdlib.h>
> > +#include <limits.h>
> > +#include <grp.h>
> > +#include <errno.h>
> > +#include <error.h>
> > +#include <support/xthread.h>
> > +#include <support/support.h>
> > +#include <support/test-driver.h>
> > +#include <support/xunistd.h>
> > +
>
> Suggest:
>
> /* The purpose of this test is to test the setgroups API as root
>    and in the presence of threads.  Once we create a thread the
>    setgroups implementation must ensure that all threads are set
>    to the same group and this operation should not fail. Lastly
>    we test setgroups with a zero sized group and a bad address
>    and verify we get EPERM.  */
Fixed.

> > +static void *
> > +start_routine (void *args)
> > +{
> > +  return NULL;
> > +}
> > +
> > +static int
> > +do_test (void)
> > +{
> > +  int size;
> > +  /* NB: Stack address is at 0xfffXXXXX.  */
> > +  gid_t list[NGROUPS_MAX];
> > +  int status = EXIT_SUCCESS;
> > +
> > +  pthread_t thread = xpthread_create (NULL, start_routine, NULL);
> > +
> > +  size = getgroups (sizeof (list) / sizeof (list[0]), list);
> > +  if (size < 0)
> > +    {
> > +      status = EXIT_FAILURE;
> > +      error (0, errno, "getgroups failed");
> > +    }
> > +  if (setgroups (size, list) < 0)
> > +    {
> > +      if (errno == EPERM)
> > +     status = EXIT_UNSUPPORTED;
> > +      else
> > +     {
> > +       status = EXIT_FAILURE;
> > +       error (0, errno, "setgroups failed");
> > +     }
> > +    }
>
>
> Test again with setgroups (0, bad addresss) ?
Fixed.

> > +
> > +  xpthread_join (thread);
> > +
> > +  return status;
> > +}
> > +
> > +#include <support/test-driver.c>
> > --
> > 2.26.2
> >
>
Here is the updated patch.

Thanks.

--
H.J.

0001-nptl-Zero-extend-arguments-to-SETXID-syscalls-BZ-262.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
In reply to this post by Sourceware - libc-alpha mailing list
On 7/17/20 3:31 PM, H.J. Lu wrote:

> On Fri, Jul 17, 2020 at 8:52 AM Carlos O'Donell <[hidden email]> wrote:
>>
>> On 7/17/20 11:13 AM, Florian Weimer wrote:
>>> * Carlos O'Donell:
>>>
>>>> This test should run in a container, and it should attempt two setgroups
>>>> calls, one with groups and one empty with a bad address.
>>>
>>> Why do you think this needs a container?
>>
>> We are trying to successfully call setgroups(), and to do that we need
>> CAP_SETGID. The way this test is exercising this is by making the test
>> an xtests which can require root and thus you get CAP_SETGID in that way.
>>
>> My suggestion is to move the test from xtests to tests-container to increase
>> the usage of the test. In the container we have a CLONE_NEWUSER so we have
>> a distinct usersnamespace that can be used in conjunction with becoming
>> root, getting CAP_SETGID, and calling setgroups() without restricting this
>> test to `make xcheck`.
>
> I see su in tst-localedef-path-norm.script.  But when I removed "su" from
> tst-localedef-path-norm.script, tst-localedef-path-norm still passed.  There are

The use "su" changes uid_map and gid_map to map our users to user 0 in the
container, but doesn't explicitly deny us from writing to the files in the
filesytem. The use of "su" in this test was belt-and-suspenders in case
some code internally checked the uid/gid values.
 

> [hjl@gnu-cfl-2 build-x86_64-linux]$ ls -l testroot.root
> total 44
> drwxr-xr-x 2 hjl hjl 4096 Jul 17 09:16 bin
> drwxr-xr-x 2 hjl hjl 4096 Jul 17 12:23 dev
> drwxr-xr-x 2 hjl hjl 4096 Jul 17 09:34 etc
> drwxr-xr-x 4 hjl hjl 4096 Jul 17 12:23 export
> -rw-r--r-- 1 hjl hjl    0 Jul 17 09:16 install.stamp
> drwxr-xr-x 2 hjl hjl 4096 Jul 17 09:16 libx32
> -rw-r--r-- 1 hjl hjl    0 Jul 17 12:26 lock.fd
> drwxr-xr-x 5 hjl hjl 4096 Jul 17 12:23 output
> drwxr-xr-x 2 hjl hjl 4096 Jul 17 12:23 proc
> drwxr-xr-x 2 hjl hjl 4096 Jul 17 09:16 sbin
> drwxr-xr-x 2 hjl hjl 4096 Jul 17 12:23 tmp
> drwxr-xr-x 9 hjl hjl 4096 Jul 17 09:16 usr
> drwxr-xr-x 3 hjl hjl 4096 Jul 17 09:34 var
> [hjl@gnu-cfl-2 build-x86_64-linux]$
>
> I don't think su is needed since testroot.root is owned by me.

Correct.

>> I see that we don't explicitly say `make xcheck` may require root.
>> Is this something I just taught myself implicitly? :-)
>>
>> Note: We may need to adjust the gid_map writing code in test-container.
>>
>
> Can you help me to make tst-setgroups pass when not running as root?
 
Sure, let me have a look at running it as a test in the container.

--
Cheers,
Carlos.

Reply | Threaded
Open this post in threaded view
|

Re: V2 [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Aurelien Jarno
In reply to this post by Sourceware - libc-alpha mailing list
On 2020-07-17 12:42, H.J. Lu via Libc-alpha wrote:

> On Fri, Jul 17, 2020 at 8:02 AM Carlos O'Donell <[hidden email]> wrote:
> >
> > On 7/16/20 8:46 AM, H.J. Lu via Libc-alpha wrote:
> > > On Thu, Jul 16, 2020 at 5:03 AM Florian Weimer <[hidden email]> wrote:
> > >>
> > >> * H. J. Lu via Libc-alpha:
> > >>
> > >>> nptl has
> > >>>
> > >>> /* Opcodes and data types for communication with the signal handler to
> > >>>    change user/group IDs.  */
> > >>> struct xid_command
> > >>> {
> > >>>   int syscall_no;
> > >>>   long int id[3];
> > >>>   volatile int cntr;
> > >>>   volatile int error;
> > >>> };
> > >>>
> > >>>  /* This must be last, otherwise the current thread might not have
> > >>>      permissions to send SIGSETXID syscall to the other threads.  */
> > >>>   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> > >>>                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> > >>>
> > >>> But the second argument of setgroups syscal is a pointer:
> > >>>
> > >>>        int setgroups(size_t size, const gid_t *list);
> > >>>
> > >>> But on x32, pointers passed to syscall must have pointer type so that they
> > >>> will be zero-extended.
> > >>>
> > >>> Add <setxid-internal.h> to define INTERNAL_SETXID_SYSCALL_NCS and use it,
> > >>> instead of INTERNAL_SYSCALL_NCS, for SETXID syscalls.  X32 override it
> > >>> with pointer type for setgroups.  A testcase is added and setgroups
> > >>> returned with EFAULT when running as root without the fix.
> > >>
> > >> Isn't it sufficient to change the type of id to unsigned long int[3]?
> > >> The UID arguments are unsigned on the kernel side, so no sign extension
> > >> is required.
> > >>
> > >
> > > It works.  Here is the updated patch.  OK for master?
> > >
> > > Thanks.
> > >
> >
> > This test should run in a container, and it should attempt two setgroups
> > calls, one with groups and one empty with a bad address.
>
> Fixed.
>
> > > From 2af9e56c2306dc9d80a4476fa5b154a26a935557 Mon Sep 17 00:00:00 2001
> > > From: "H.J. Lu" <[hidden email]>
> > > Date: Thu, 16 Jul 2020 03:37:10 -0700
> > > Subject: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]
> > >
> > > nptl has
> > >
> > > /* Opcodes and data types for communication with the signal handler to
> > >    change user/group IDs.  */
> > > struct xid_command
> > > {
> > >   int syscall_no;
> > >   long int id[3];
> > >   volatile int cntr;
> > >   volatile int error;
> > > };
> > >
> > >  /* This must be last, otherwise the current thread might not have
> > >      permissions to send SIGSETXID syscall to the other threads.  */
> > >   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> > >                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> > >
> > > But the second argument of setgroups syscal is a pointer:
> > >
> > >        int setgroups(size_t size, const gid_t *list);
> > >
> > > But on x32, pointers passed to syscall must have pointer type so that they
> > > will be zero-extended.  Since the XID arguments are unsigned on the kernel
> > > side, so no sign extension is required.  Change xid_command to
> > >
> > > struct xid_command
> > > {
> > >   int syscall_no;
> > >   unsigned long int id[3];
> > >   volatile int cntr;
> > >   volatile int error;
> > > };
> > >
> > > so that all arguments zero-extended.  A testcase is added for x32 and
> > > setgroups returned with EFAULT when running as root without the fix.
> > > ---
> > >  nptl/descr.h                                  |  8 ++-
> > >  sysdeps/unix/sysv/linux/x86_64/x32/Makefile   |  4 ++
> > >  .../sysv/linux/x86_64/x32/tst-setgroups.c     | 67 +++++++++++++++++++
> > >  3 files changed, 78 insertions(+), 1 deletion(-)
> > >  create mode 100644 sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c

Is there a reason to limit that test to x32? We do not have any other
getgroups or setgroups test in the GLIBC tree, so that simple test might
already be able to catch issues. In addition such a test would benefit
the other ILP32 architectures supported by GLIBC.

Aurelien

--
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[hidden email]                 http://www.aurel32.net
Reply | Threaded
Open this post in threaded view
|

Re: V2 [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
On Fri, Jul 17, 2020 at 3:27 PM Aurelien Jarno <[hidden email]> wrote:

>
> On 2020-07-17 12:42, H.J. Lu via Libc-alpha wrote:
> > On Fri, Jul 17, 2020 at 8:02 AM Carlos O'Donell <[hidden email]> wrote:
> > >
> > > On 7/16/20 8:46 AM, H.J. Lu via Libc-alpha wrote:
> > > > On Thu, Jul 16, 2020 at 5:03 AM Florian Weimer <[hidden email]> wrote:
> > > >>
> > > >> * H. J. Lu via Libc-alpha:
> > > >>
> > > >>> nptl has
> > > >>>
> > > >>> /* Opcodes and data types for communication with the signal handler to
> > > >>>    change user/group IDs.  */
> > > >>> struct xid_command
> > > >>> {
> > > >>>   int syscall_no;
> > > >>>   long int id[3];
> > > >>>   volatile int cntr;
> > > >>>   volatile int error;
> > > >>> };
> > > >>>
> > > >>>  /* This must be last, otherwise the current thread might not have
> > > >>>      permissions to send SIGSETXID syscall to the other threads.  */
> > > >>>   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> > > >>>                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> > > >>>
> > > >>> But the second argument of setgroups syscal is a pointer:
> > > >>>
> > > >>>        int setgroups(size_t size, const gid_t *list);
> > > >>>
> > > >>> But on x32, pointers passed to syscall must have pointer type so that they
> > > >>> will be zero-extended.
> > > >>>
> > > >>> Add <setxid-internal.h> to define INTERNAL_SETXID_SYSCALL_NCS and use it,
> > > >>> instead of INTERNAL_SYSCALL_NCS, for SETXID syscalls.  X32 override it
> > > >>> with pointer type for setgroups.  A testcase is added and setgroups
> > > >>> returned with EFAULT when running as root without the fix.
> > > >>
> > > >> Isn't it sufficient to change the type of id to unsigned long int[3]?
> > > >> The UID arguments are unsigned on the kernel side, so no sign extension
> > > >> is required.
> > > >>
> > > >
> > > > It works.  Here is the updated patch.  OK for master?
> > > >
> > > > Thanks.
> > > >
> > >
> > > This test should run in a container, and it should attempt two setgroups
> > > calls, one with groups and one empty with a bad address.
> >
> > Fixed.
> >
> > > > From 2af9e56c2306dc9d80a4476fa5b154a26a935557 Mon Sep 17 00:00:00 2001
> > > > From: "H.J. Lu" <[hidden email]>
> > > > Date: Thu, 16 Jul 2020 03:37:10 -0700
> > > > Subject: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]
> > > >
> > > > nptl has
> > > >
> > > > /* Opcodes and data types for communication with the signal handler to
> > > >    change user/group IDs.  */
> > > > struct xid_command
> > > > {
> > > >   int syscall_no;
> > > >   long int id[3];
> > > >   volatile int cntr;
> > > >   volatile int error;
> > > > };
> > > >
> > > >  /* This must be last, otherwise the current thread might not have
> > > >      permissions to send SIGSETXID syscall to the other threads.  */
> > > >   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> > > >                                  cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> > > >
> > > > But the second argument of setgroups syscal is a pointer:
> > > >
> > > >        int setgroups(size_t size, const gid_t *list);
> > > >
> > > > But on x32, pointers passed to syscall must have pointer type so that they
> > > > will be zero-extended.  Since the XID arguments are unsigned on the kernel
> > > > side, so no sign extension is required.  Change xid_command to
> > > >
> > > > struct xid_command
> > > > {
> > > >   int syscall_no;
> > > >   unsigned long int id[3];
> > > >   volatile int cntr;
> > > >   volatile int error;
> > > > };
> > > >
> > > > so that all arguments zero-extended.  A testcase is added for x32 and
> > > > setgroups returned with EFAULT when running as root without the fix.
> > > > ---
> > > >  nptl/descr.h                                  |  8 ++-
> > > >  sysdeps/unix/sysv/linux/x86_64/x32/Makefile   |  4 ++
> > > >  .../sysv/linux/x86_64/x32/tst-setgroups.c     | 67 +++++++++++++++++++
> > > >  3 files changed, 78 insertions(+), 1 deletion(-)
> > > >  create mode 100644 sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
>
> Is there a reason to limit that test to x32? We do not have any other
> getgroups or setgroups test in the GLIBC tree, so that simple test might
> already be able to catch issues. In addition such a test would benefit
> the other ILP32 architectures supported by GLIBC.
>

I can move it to nptl.

--
H.J.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
In reply to this post by Sourceware - libc-alpha mailing list
* Carlos O'Donell:

> On 7/17/20 11:13 AM, Florian Weimer wrote:
>> * Carlos O'Donell:
>>
>>> This test should run in a container, and it should attempt two setgroups
>>> calls, one with groups and one empty with a bad address.
>>
>> Why do you think this needs a container?
>
> We are trying to successfully call setgroups(), and to do that we need
> CAP_SETGID.

Hmm, I think you are right: Since group membership can be used to
restrict privileges, removing supplementary groups is itself a
privileged call.

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

Sourceware - libc-alpha mailing list
In reply to this post by Sourceware - libc-alpha mailing list
On Fri, Jul 17, 2020 at 2:22 PM Carlos O'Donell <[hidden email]> wrote:

>
> On 7/17/20 3:31 PM, H.J. Lu wrote:
> > On Fri, Jul 17, 2020 at 8:52 AM Carlos O'Donell <[hidden email]> wrote:
> >>
> >> On 7/17/20 11:13 AM, Florian Weimer wrote:
> >>> * Carlos O'Donell:
> >>>
> >>>> This test should run in a container, and it should attempt two setgroups
> >>>> calls, one with groups and one empty with a bad address.
> >>>
> >>> Why do you think this needs a container?
> >>
> >> We are trying to successfully call setgroups(), and to do that we need
> >> CAP_SETGID. The way this test is exercising this is by making the test
> >> an xtests which can require root and thus you get CAP_SETGID in that way.
> >>
> >> My suggestion is to move the test from xtests to tests-container to increase
> >> the usage of the test. In the container we have a CLONE_NEWUSER so we have
> >> a distinct usersnamespace that can be used in conjunction with becoming
> >> root, getting CAP_SETGID, and calling setgroups() without restricting this
> >> test to `make xcheck`.
> >
> > I see su in tst-localedef-path-norm.script.  But when I removed "su" from
> > tst-localedef-path-norm.script, tst-localedef-path-norm still passed.  There are
>
> The use "su" changes uid_map and gid_map to map our users to user 0 in the
> container, but doesn't explicitly deny us from writing to the files in the
> filesytem. The use of "su" in this test was belt-and-suspenders in case
> some code internally checked the uid/gid values.
>
> > [hjl@gnu-cfl-2 build-x86_64-linux]$ ls -l testroot.root
> > total 44
> > drwxr-xr-x 2 hjl hjl 4096 Jul 17 09:16 bin
> > drwxr-xr-x 2 hjl hjl 4096 Jul 17 12:23 dev
> > drwxr-xr-x 2 hjl hjl 4096 Jul 17 09:34 etc
> > drwxr-xr-x 4 hjl hjl 4096 Jul 17 12:23 export
> > -rw-r--r-- 1 hjl hjl    0 Jul 17 09:16 install.stamp
> > drwxr-xr-x 2 hjl hjl 4096 Jul 17 09:16 libx32
> > -rw-r--r-- 1 hjl hjl    0 Jul 17 12:26 lock.fd
> > drwxr-xr-x 5 hjl hjl 4096 Jul 17 12:23 output
> > drwxr-xr-x 2 hjl hjl 4096 Jul 17 12:23 proc
> > drwxr-xr-x 2 hjl hjl 4096 Jul 17 09:16 sbin
> > drwxr-xr-x 2 hjl hjl 4096 Jul 17 12:23 tmp
> > drwxr-xr-x 9 hjl hjl 4096 Jul 17 09:16 usr
> > drwxr-xr-x 3 hjl hjl 4096 Jul 17 09:34 var
> > [hjl@gnu-cfl-2 build-x86_64-linux]$
> >
> > I don't think su is needed since testroot.root is owned by me.
>
> Correct.
>
> >> I see that we don't explicitly say `make xcheck` may require root.
> >> Is this something I just taught myself implicitly? :-)
> >>
> >> Note: We may need to adjust the gid_map writing code in test-container.
> >>
> >
> > Can you help me to make tst-setgroups pass when not running as root?
>
> Sure, let me have a look at running it as a test in the container.
>

Any update on this?


--
H.J.
12