From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-2.mimecast.com [207.211.31.81]) by sourceware.org (Postfix) with ESMTP id B90963857C52 for ; Fri, 17 Jul 2020 02:14:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B90963857C52 Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-73-tPqbRMJQOS2x-W7hfQAX3g-1; Thu, 16 Jul 2020 22:14:33 -0400 X-MC-Unique: tPqbRMJQOS2x-W7hfQAX3g-1 Received: by mail-qt1-f199.google.com with SMTP id c26so5228280qtq.6 for ; Thu, 16 Jul 2020 19:14:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=MWTQS4Pd0zRSPg0ipq8ilnyxuRV5vfG0NA9F3ycW9CM=; b=BiSUSPXC5FvYGlfLXLQGD9ncUs7h+PbBMgC/sCe+0WGPz0QCF7c1wjWeQg8E09Uppg kydTT+01NtUBrYwfNaNcyNxzoKpIszL+JmZwPSEM9RqQF4f3pIkzjbz9P9Byf/Xw+A6V 6dFbKy/iyxAVIwSx/F1kW1vyPO6cx1R8byAhoT4mJVaf2zJh7mrZ1DrR4mehwrqJoyLU /hp2mrHtQVPhfPTTTctgIXchdO+hPC8BSeHKRNosN3ICMDKOVWwBkIixnULA9aQRjr4a 67cPNV0YGIx57xL60MvBqgfLvZrX6F3RHRMFmlEDP5eo1hG06zpr3q7WjWG2T0fRmDU2 qp+Q== X-Gm-Message-State: AOAM531+rTU99cjTEiXvxC/FUYL8iw16OQcPEG3xn1MjKQH7wnKxQMhO Ot7LK7aNykKxHEHSc4/TDTWyWUL0eveFsJxWsOD8Y3tIN3cuTXO7FgKBdxf/SIoQsSzVbMf9qqo JSpowKAe0QKgDdRaUZ64U X-Received: by 2002:a37:4050:: with SMTP id n77mr6804809qka.431.1594952072263; Thu, 16 Jul 2020 19:14:32 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzDFhR6BTUAFrQGmIGKKHM6iz8oZCkmxFWCvdAtu1Rw+/n5Mdvg1zqPY5GnVGYwASEjXnxYPA== X-Received: by 2002:a37:4050:: with SMTP id n77mr6804791qka.431.1594952071930; Thu, 16 Jul 2020 19:14:31 -0700 (PDT) Received: from [192.168.1.4] (198-84-170-103.cpe.teksavvy.com. [198.84.170.103]) by smtp.gmail.com with ESMTPSA id z18sm10065474qta.51.2020.07.16.19.14.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 16 Jul 2020 19:14:31 -0700 (PDT) Subject: Re: [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248] To: "H.J. Lu" , Florian Weimer , "H.J. Lu via Libc-alpha" References: <20200716112651.2257283-1-hjl.tools@gmail.com> <87o8ofy8e7.fsf@oldenburg2.str.redhat.com> <20200716194557.GC8268@aurel32.net> From: Carlos O'Donell Organization: Red Hat Message-ID: Date: Thu, 16 Jul 2020 22:14:25 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, 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: Fri, 17 Jul 2020 02:14:39 -0000 On 7/16/20 5:42 PM, H.J. Lu wrote: > 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. Please point me at the final version you want reviewed and I'll do that first thing tomorrow morning. -- Cheers, Carlos.