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

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

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.
  

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
  

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
  

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.
  

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.
  

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
  

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
>
  

* 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
  

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.
  

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?
  

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.
  

* 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
  

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?
  

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.
  

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

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

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.
  

* 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
  

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

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

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.