Re: V2 [PATCH] nptl: Zero-extend arguments to SETXID syscalls [BZ #26248]

[Aurelien Jarno <aurelien@aurel32.net>]

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

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net