From patchwork Fri Jul 17 19:42:37 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "H.J. Lu" X-Patchwork-Id: 40135 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7A352393BC16; Fri, 17 Jul 2020 19:43:17 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7A352393BC16 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1595014997; bh=dh61T8MVqj+m6QCQkNCS3zwVcDgq9DEJEvmekEt5nS8=; h=References:In-Reply-To:Date:Subject:To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=oqGAw780niv/RkqeOaqu0yZdhazaVf4dwB0V2gGhL/VOu7ws4l0UvX0ZE+rtTxHfp BUp4xxtpzP5MNx8PHrQDwc2CUSwvuoxQ4cFaKesf55jxCZajmmKCZP2xS1VeU/XdB9 YUauHTR5SI3v/vLF4oYdA03NQ54lwVoBnUb4oDCc= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-io1-xd43.google.com (mail-io1-xd43.google.com [IPv6:2607:f8b0:4864:20::d43]) by sourceware.org (Postfix) with ESMTPS id 47EE03857028 for ; Fri, 17 Jul 2020 19:43:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 47EE03857028 Received: by mail-io1-xd43.google.com with SMTP id d18so11628617ion.0 for ; Fri, 17 Jul 2020 12:43:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=dh61T8MVqj+m6QCQkNCS3zwVcDgq9DEJEvmekEt5nS8=; b=JydwOdWg/xQ7vr1q+x44jKMUrqWfO9Gv3yqU5Hx0op/Vmb1HNRCVJNJQetv+LCG/Dc kq1h23ujB+zntp4Hs5Za9EbRj21LRk66gm1WUwTVhlFkPweE0eacLAYnkqb2ft9woYp1 s2t+/IvJY8GXXp3fqOcNDaaSaQSkte/4z4I4jk9FwQPveew5KQ/yrDoJINt7tXPYJ5vq JuRHwjs8NKs1gNp9daLn93MH1BfGZT0JdC5XDK7Ddma2uWkaP9CgzaHqBlM/ZPuGVyTC Jr3kyx3tuWli7B4cqMi+r8lYu5zr/+4+a0bGjz+q3scCpmor5muLC+1QMRpcmIkaTKk8 2fmQ== X-Gm-Message-State: AOAM530Vh3y3w7ir1/B7zcnKrmRnoL+3Zih/R/ipYXJkFVUIuiH23/sA NfaYDyDCdueBt/Gqe8sSPoxGrg6K0UuLgWa4lXo= X-Google-Smtp-Source: ABdhPJwClw/pmRmqj3XWSv9lOJL59LgiJI5ZcQGliECig2j1AfP9P55YHbApKoJqYkPpc0226MdYdONtmG6HarB6eFg= X-Received: by 2002:a05:6638:1187:: with SMTP id f7mr12785758jas.58.1595014993671; Fri, 17 Jul 2020 12:43:13 -0700 (PDT) MIME-Version: 1.0 References: <20200716112651.2257283-1-hjl.tools@gmail.com> <87o8ofy8e7.fsf@oldenburg2.str.redhat.com> <56cafa21-37ea-b39e-8c84-afb258f0d17a@redhat.com> In-Reply-To: <56cafa21-37ea-b39e-8c84-afb258f0d17a@redhat.com> Date: Fri, 17 Jul 2020 12:42:37 -0700 Message-ID: Subject: V2 [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248] To: "Carlos O'Donell" X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: "H.J. Lu via Libc-alpha" From: "H.J. Lu" Reply-To: "H.J. Lu" Cc: Florian Weimer , "H.J. Lu via Libc-alpha" Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" 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. From ba75444ae218bc590a9d3a49aa538ad089a0161a 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. 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 + . */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* 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 -- 2.26.2