[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