[PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]
Carlos O'Donell
carlos@redhat.com
Fri Jul 17 15:01:40 GMT 2020
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.
> 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);
> +
Suggest:
The kernel XID arguments are unsigned and do not require sign extension.
> + 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.
> +
> 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)
> + 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>
> +
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. */
> +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) ?
> +
> + xpthread_join (thread);
> +
> + return status;
> +}
> +
> +#include <support/test-driver.c>
> --
> 2.26.2
>
--
Cheers,
Carlos.
More information about the Libc-alpha
mailing list