[PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]
Carlos O'Donell
carlos@redhat.com
Fri Jul 17 02:14:25 GMT 2020
On 7/16/20 5:42 PM, H.J. Lu wrote:
> On Thu, Jul 16, 2020 at 12:45 PM Aurelien Jarno <aurelien@aurel32.net> 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 <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.
>>>
>>> --
>>> H.J.
>>
>>> 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
>>>
>>> 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
>>> + <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>
>>> +
>>> +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.
More information about the Libc-alpha
mailing list