From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 355BD389851C for ; Thu, 16 Jul 2020 21:43:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 355BD389851C Received: by mail-io1-xd43.google.com with SMTP id f23so7736844iof.6 for ; Thu, 16 Jul 2020 14:43:20 -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=DVOhPkGWb2R6yTcqfdI4WLGvJ6Oqbi+EpyZap9w96MQ=; b=VZ/t32ph/a6bkYskZgHateSvbo+uNePiRgsqhVuqKmDUmdyXCIescd1y2Q/s2qqOtZ pLzanpIAENLkAW3a2fmz0/8KSRHnqUivrIr/sl2Ib8xkc22koB8zWZAEXnwID5KKVF2r J3r1r9zOhX/wbSjHBq+VHrl7GqcHgHd7Obz4qMLWZpgo9lTORwct8qhTxKwx9qzSyMbt iGywCyEhqP79IzlSVAYNXVN/L20+Jgm/sSAJwq3R31fTSv4We6bjfXiP6/jdVwOdQIVw PT8UsHYbvBBr68YoN6BKkiPO+XqIAMGfnnNvt6CmWgqUEvppXRyzlQKm6e0BFQOYhV4b eKIw== X-Gm-Message-State: AOAM531F+46R2ODnKOuDWJ6pAggoyuAicgn3zIg6tVXS3qAqnQ2E1jZw 7EhKTfmJ80KxttnY0Ju/5ZCx9q0TEHygtYBXhLM= X-Google-Smtp-Source: ABdhPJyi8G8w2X36YNCy1O4tCvG1CCqdJdiWwBqUVMHI1hjjleAn94g8A+zIHliDJHMKQbzMNYa5UhkAfOAof+pCKEg= X-Received: by 2002:a92:cf42:: with SMTP id c2mr6984213ilr.13.1594935799533; Thu, 16 Jul 2020 14:43:19 -0700 (PDT) MIME-Version: 1.0 References: <20200716112651.2257283-1-hjl.tools@gmail.com> <87o8ofy8e7.fsf@oldenburg2.str.redhat.com> <20200716194557.GC8268@aurel32.net> In-Reply-To: <20200716194557.GC8268@aurel32.net> From: "H.J. Lu" Date: Thu, 16 Jul 2020 14:42:43 -0700 Message-ID: Subject: Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248] To: Florian Weimer , "H.J. Lu via Libc-alpha" Content-Type: text/plain; charset="UTF-8" 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-List-Received-Date: Thu, 16 Jul 2020 21:43:21 -0000 On Thu, Jul 16, 2020 at 12:45 PM Aurelien Jarno 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 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. > > > > -- > > H.J. > > > 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); > > + > > + 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 > > + . */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +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.