V2 [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]
Commit Message
On Fri, Jul 17, 2020 at 8:02 AM Carlos O'Donell <carlos@redhat.com> 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 <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.
Fixed.
> > 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.
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
> > + <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. */
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 <support/test-driver.c>
> > --
> > 2.26.2
> >
>
Here is the updated patch.
Thanks.
--
H.J.
Comments
On 2020-07-17 12:42, H.J. Lu via Libc-alpha wrote:
> On Fri, Jul 17, 2020 at 8:02 AM Carlos O'Donell <carlos@redhat.com> 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 <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.
>
> Fixed.
>
> > > 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
Is there a reason to limit that test to x32? We do not have any other
getgroups or setgroups test in the GLIBC tree, so that simple test might
already be able to catch issues. In addition such a test would benefit
the other ILP32 architectures supported by GLIBC.
Aurelien
On Fri, Jul 17, 2020 at 3:27 PM Aurelien Jarno <aurelien@aurel32.net> wrote:
>
> On 2020-07-17 12:42, H.J. Lu via Libc-alpha wrote:
> > On Fri, Jul 17, 2020 at 8:02 AM Carlos O'Donell <carlos@redhat.com> 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 <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.
> >
> > Fixed.
> >
> > > > 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
>
> Is there a reason to limit that test to x32? We do not have any other
> getgroups or setgroups test in the GLIBC tree, so that simple test might
> already be able to catch issues. In addition such a test would benefit
> the other ILP32 architectures supported by GLIBC.
>
I can move it to nptl.
From ba75444ae218bc590a9d3a49aa538ad089a0161a 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. 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
@@ -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. */
};
@@ -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
new file mode 100644
@@ -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
+ <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>
+
+/* 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 <support/test-driver.c>
--
2.26.2