On Fri, Jul 17, 2020 at 8:02 AM Carlos O'Donell 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 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 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" > > 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 > > + . */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > 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 > > -- > > 2.26.2 > > > Here is the updated patch. Thanks. -- H.J.