[PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]
H.J. Lu
hjl.tools@gmail.com
Thu Jul 16 21:42:43 GMT 2020
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.
More information about the Libc-alpha
mailing list