Message ID | CAMe9rOrR2f5xSj-FzN6Tk1-5BqKPTS1ZsqJEUcZFjJVHRoZjyw@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | V2 [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248] | expand |
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
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.
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