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

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

[H.J. Lu]

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

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

[Florian Weimer]

* 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

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1417 bytes --]

On Thu, Jul 16, 2020 at 5:03 AM Florian Weimer <fweimer@redhat.com> 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.

[-- Attachment #2: 0001-nptl-Zero-extend-arguments-to-SETXID-syscalls-BZ-262.patch --]
[-- Type: text/x-patch, Size: 4703 bytes --]

From 2af9e56c2306dc9d80a4476fa5b154a26a935557 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
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)
+    {
+      if (errno == EPERM)
+	status = EXIT_UNSUPPORTED;
+      else
+	{
+	  status = EXIT_FAILURE;
+	  error (0, errno, "setgroups failed");
+	}
+    }
+
+  xpthread_join (thread);
+
+  return status;
+}
+
+#include <support/test-driver.c>
-- 
2.26.2

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

[Florian Weimer]

* 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

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

[H.J. Lu]

On Thu, Jul 16, 2020 at 8:57 AM Florian Weimer <fweimer@redhat.com> 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.

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

[Aurelien Jarno]

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
aurelien@aurel32.net                 http://www.aurel32.net

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

[Aurelien Jarno]

On 2020-07-16 05:46, H.J. Lu via Libc-alpha wrote:
> On Thu, Jul 16, 2020 at 5:03 AM Florian Weimer <fweimer@redhat.com> 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" <hjl.tools@gmail.com>
> 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
aurelien@aurel32.net                 http://www.aurel32.net

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

[H.J. Lu]

On Thu, Jul 16, 2020 at 12:45 PM Aurelien Jarno <aurelien@aurel32.net> 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 <fweimer@redhat.com> 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" <hjl.tools@gmail.com>
> > 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.

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

[Carlos O'Donell]

On 7/16/20 5:42 PM, H.J. Lu wrote:
> On Thu, Jul 16, 2020 at 12:45 PM Aurelien Jarno <aurelien@aurel32.net> 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 <fweimer@redhat.com> 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" <hjl.tools@gmail.com>
>>> 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.

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

[H.J. Lu]

On Thu, Jul 16, 2020 at 7:14 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 7/16/20 5:42 PM, H.J. Lu wrote:
> > On Thu, Jul 16, 2020 at 12:45 PM Aurelien Jarno <aurelien@aurel32.net> 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 <fweimer@redhat.com> 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" <hjl.tools@gmail.com>
> >>> 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.

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

[Carlos O'Donell]

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 <fweimer@redhat.com> 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" <hjl.tools@gmail.com>
> 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.

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

[Florian Weimer]

* 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

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

[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. 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.

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

[H.J. Lu]

On Fri, Jul 17, 2020 at 8:52 AM Carlos O'Donell <carlos@redhat.com> 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.

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 8034 bytes --]

On Fri, Jul 17, 2020 at 8:02 AM Carlos O'Donell <carlos@redhat.com> 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 <fweimer@redhat.com> 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" <hjl.tools@gmail.com>
> > 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.

[-- Attachment #2: 0001-nptl-Zero-extend-arguments-to-SETXID-syscalls-BZ-262.patch --]
[-- Type: text/x-patch, Size: 5216 bytes --]

From ba75444ae218bc590a9d3a49aa538ad089a0161a Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
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.  The kernel XID arguments are unsigned and
do not require sign extension.  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 are 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     | 79 +++++++++++++++++++
 3 files changed, 90 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..a0fc3fda0f 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);
+
+     The kernel XID arguments are unsigned and do not require sign
+     extension.  */
+  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..1ebd0b4cff
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
@@ -0,0 +1,79 @@
+/* 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>
+
+/* 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");
+	}
+    }
+
+  if (status == EXIT_SUCCESS && setgroups (0, list) < 0)
+    {
+      status = EXIT_FAILURE;
+      error (0, errno, "setgroups failed");
+    }
+
+  xpthread_join (thread);
+
+  return status;
+}
+
+#include <support/test-driver.c>
-- 
2.26.2

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

[Carlos O'Donell]

On 7/17/20 3:31 PM, H.J. Lu wrote:
> On Fri, Jul 17, 2020 at 8:52 AM Carlos O'Donell <carlos@redhat.com> 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.

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

[Aurelien Jarno]

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 <carlos@redhat.com> 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 <fweimer@redhat.com> 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" <hjl.tools@gmail.com>
> > > 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
aurelien@aurel32.net                 http://www.aurel32.net

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

[H.J. Lu]

On Fri, Jul 17, 2020 at 3:27 PM Aurelien Jarno <aurelien@aurel32.net> 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 <carlos@redhat.com> 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 <fweimer@redhat.com> 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" <hjl.tools@gmail.com>
> > > > 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.

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

[Florian Weimer]

* 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

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

[H.J. Lu]

On Fri, Jul 17, 2020 at 2:22 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 7/17/20 3:31 PM, H.J. Lu wrote:
> > On Fri, Jul 17, 2020 at 8:52 AM Carlos O'Donell <carlos@redhat.com> 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.

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

[Carlos O'Donell]

On 7/23/20 4:03 PM, H.J. Lu wrote:
> On Fri, Jul 17, 2020 at 2:22 PM Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> On 7/17/20 3:31 PM, H.J. Lu wrote:
>>> On Fri, Jul 17, 2020 at 8:52 AM Carlos O'Donell <carlos@redhat.com> 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?

Not yet, I have 3 things on my plate:

* AArch64 PAC+BTI enablement review.
* nptl: Zero-extend arguments to SETXID syscalls.
* FAIL: elf/tst-ldconfig-ld_so_conf-update

Once I get the first done I'm going to work on this.

If you can reproduce the last one on Fedora 32 that would
be great. I don't know what the problem is.

-- 
Cheers,
Carlos.

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

[Adhemerval Zanella]

On 23/07/2020 18:11, Carlos O'Donell via Libc-alpha wrote:
> 
> * AArch64 PAC+BTI enablement review.

Btw, what is it missing for AArch64 PAC+BTI?

> * nptl: Zero-extend arguments to SETXID syscalls.
> * FAIL: elf/tst-ldconfig-ld_so_conf-update
> 
> Once I get the first done I'm going to work on this.
> 
> If you can reproduce the last one on Fedora 32 that would
> be great. I don't know what the problem is.
> 

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

[Carlos O'Donell]

On 7/23/20 5:17 PM, Adhemerval Zanella wrote:
> 
> 
> On 23/07/2020 18:11, Carlos O'Donell via Libc-alpha wrote:
>>
>> * AArch64 PAC+BTI enablement review.
> 
> Btw, what is it missing for AArch64 PAC+BTI?

I don't know. I'm reviewing the Fedora enablement to make sure
it doesn't blow up :-)
 
>> * nptl: Zero-extend arguments to SETXID syscalls.
>> * FAIL: elf/tst-ldconfig-ld_so_conf-update
>>
>> Once I get the first done I'm going to work on this.
>>
>> If you can reproduce the last one on Fedora 32 that would
>> be great. I don't know what the problem is.
>>
> 


-- 
Cheers,
Carlos.

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

[Carlos O'Donell]

On 7/23/20 5:11 PM, Carlos O'Donell wrote:
> On 7/23/20 4:03 PM, H.J. Lu wrote:
>> On Fri, Jul 17, 2020 at 2:22 PM Carlos O'Donell <carlos@redhat.com> wrote:
>>>
>>> On 7/17/20 3:31 PM, H.J. Lu wrote:
>>>> On Fri, Jul 17, 2020 at 8:52 AM Carlos O'Donell <carlos@redhat.com> 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?
> 
> Not yet, I have 3 things on my plate:
> 
> * AArch64 PAC+BTI enablement review.
> * nptl: Zero-extend arguments to SETXID syscalls.
> * FAIL: elf/tst-ldconfig-ld_so_conf-update
> 
> Once I get the first done I'm going to work on this.
> 
> If you can reproduce the last one on Fedora 32 that would
> be great. I don't know what the problem is.
 
We can't fix this easily quickly.

I just reviewed this and we can't easily call setgroups() in a
container test without first enhancing test-container().

The xtests test will have to be good enough for now. Thank you.

-- 
Cheers,
Carlos.

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

[Florian Weimer]

* Carlos O'Donell via Libc-alpha:

> We can't fix this easily quickly.

We can make this test a non-container xtest today.  It's better than no
test at all.

Thanks,
Florian

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

[H.J. Lu]

On Sun, Jul 26, 2020 at 11:00 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Carlos O'Donell via Libc-alpha:
>
> > We can't fix this easily quickly.
>
> We can make this test a non-container xtest today.  It's better than no
> test at all.
>

Currently this test is in sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
since there are

static int
do_test (void)
{
  int size;
  /* NB: Stack address is at 0xfffXXXXX.  */
  gid_t list[NGROUPS_MAX];
  int status = EXIT_SUCCESS;

and x32 stack starts at 0xfffXXXXX, which triggers this bug.  Should
it be moved to nptl/tst-setgroups.c?

-- 
H.J.

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

[Florian Weimer]

* H. J. Lu:

> On Sun, Jul 26, 2020 at 11:00 PM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Carlos O'Donell via Libc-alpha:
>>
>> > We can't fix this easily quickly.
>>
>> We can make this test a non-container xtest today.  It's better than no
>> test at all.
>>
>
> Currently this test is in sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> since there are
>
> static int
> do_test (void)
> {
>   int size;
>   /* NB: Stack address is at 0xfffXXXXX.  */
>   gid_t list[NGROUPS_MAX];
>   int status = EXIT_SUCCESS;
>
> and x32 stack starts at 0xfffXXXXX, which triggers this bug.  Should
> it be moved to nptl/tst-setgroups.c?

Yes, this would make sense to me.

Thanks,
Florian

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]

On Mon, Jul 27, 2020 at 5:20 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Sun, Jul 26, 2020 at 11:00 PM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * Carlos O'Donell via Libc-alpha:
> >>
> >> > We can't fix this easily quickly.
> >>
> >> We can make this test a non-container xtest today.  It's better than no
> >> test at all.
> >>
> >
> > Currently this test is in sysdeps/unix/sysv/linux/x86_64/x32/tst-setgroups.c
> > since there are
> >
> > static int
> > do_test (void)
> > {
> >   int size;
> >   /* NB: Stack address is at 0xfffXXXXX.  */
> >   gid_t list[NGROUPS_MAX];
> >   int status = EXIT_SUCCESS;
> >
> > and x32 stack starts at 0xfffXXXXX, which triggers this bug.  Should
> > it be moved to nptl/tst-setgroups.c?
>
> Yes, this would make sense to me.
>

Here is the updated patch to add nptl/tst-setgroups.c.  OK for
master?

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-nptl-Zero-extend-arguments-to-SETXID-syscalls-BZ-262.patch --]
[-- Type: application/x-patch, Size: 5104 bytes --]

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

[Florian Weimer]

* H. J. Lu via Libc-alpha:

> +  /* Enforce zero-extension for the pointer argument in
> +
> +     int setgroups(size_t size, const gid_t *list);
> +
> +     The kernel XID arguments are unsigned and do not require sign
> +     extension.  */
> +  unsigned long int id[3];

Maybe add the missing space before '('?

Rest looks good to me for glibc 2.32.

Thanks,
Florian

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

On Mon, Jul 27, 2020 at 8:49 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu via Libc-alpha:
>
> > +  /* Enforce zero-extension for the pointer argument in
> > +
> > +     int setgroups(size_t size, const gid_t *list);
> > +
> > +     The kernel XID arguments are unsigned and do not require sign
> > +     extension.  */
> > +  unsigned long int id[3];
>
> Maybe add the missing space before '('?

Fixed.

> Rest looks good to me for glibc 2.32.
>

This is the patch I am checking in.

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-nptl-Zero-extend-arguments-to-SETXID-syscalls-BZ-262.patch --]
[-- Type: text/x-patch, Size: 5105 bytes --]

From e99cc6da913b96266dcd20402fbed4d50e060e3c Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
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.  The kernel XID arguments are unsigned and
do not require sign extension.  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 are zero-extended.  A testcase is added for x32 and
setgroups returned with EFAULT when running as root without the fix.
---
 nptl/Makefile        |  2 +-
 nptl/descr.h         |  8 ++++-
 nptl/tst-setgroups.c | 79 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 2 deletions(-)
 create mode 100644 nptl/tst-setgroups.c

diff --git a/nptl/Makefile b/nptl/Makefile
index 5e62c77853..89569c4f46 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -310,7 +310,7 @@ tests-internal := tst-robustpi8 tst-rwlock19 tst-rwlock20 \
 		  tst-setgetname \
 
 xtests = tst-setuid1 tst-setuid1-static tst-setuid2 \
-	tst-mutexpp1 tst-mutexpp6 tst-mutexpp10
+	tst-mutexpp1 tst-mutexpp6 tst-mutexpp10 tst-setgroups
 
 # This test can run into task limits because of a linux kernel bug
 # and then cause the make process to fail too, see bug 24537.
diff --git a/nptl/descr.h b/nptl/descr.h
index 6a509b6725..d8343ff9a1 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);
+
+     The kernel XID arguments are unsigned and do not require sign
+     extension.  */
+  unsigned long int id[3];
   volatile int cntr;
   volatile int error; /* -1: no call yet, 0: success seen, >0: error seen.  */
 };
diff --git a/nptl/tst-setgroups.c b/nptl/tst-setgroups.c
new file mode 100644
index 0000000000..ae3c1b1ba0
--- /dev/null
+++ b/nptl/tst-setgroups.c
@@ -0,0 +1,79 @@
+/* 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>
+
+/* 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 can be at 0xfffXXXXX on 32-bit OSes.  */
+  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");
+	}
+    }
+
+  if (status == EXIT_SUCCESS && setgroups (0, list) < 0)
+    {
+      status = EXIT_FAILURE;
+      error (0, errno, "setgroups failed");
+    }
+
+  xpthread_join (thread);
+
+  return status;
+}
+
+#include <support/test-driver.c>
-- 
2.26.2