Re: [PATCH 3/4] tst-pidfd.c: Test is UNSUPPORTED without PTRACE_MODE_ATTACH_REALCREDS

[Christian Brauner <brauner@kernel.org>]

On Mon, Jun 27, 2022 at 04:42:32PM +0200, Florian Weimer wrote:
> * Christian Brauner:
> 
> > On Mon, Jun 27, 2022 at 01:14:06PM +0200, Florian Weimer via Libc-alpha wrote:
> >> * Mark Wielaard:
> >> 
> >> > Hi Florian,
> >> >
> >> > On Sun, 2022-06-26 at 23:20 +0200, Florian Weimer wrote:
> >> >> * Mark Wielaard:
> >> >> 
> >> >> > pidfd_getfd will fail with errno EPERM if the calling process did
> >> >> > not
> >> >> > have PTRACE_MODE_ATTACH_REALCREDS permissions. Use FAIL_UNSUPPORTED
> >> >> > in that case.
> >> >> > ---
> >> >> >  sysdeps/unix/sysv/linux/tst-pidfd.c | 6 ++++--
> >> >> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >> >> > 
> >> >> > diff --git a/sysdeps/unix/sysv/linux/tst-pidfd.c
> >> >> > b/sysdeps/unix/sysv/linux/tst-pidfd.c
> >> >> > index d93b6faa6f..28349b2f91 100644
> >> >> > --- a/sysdeps/unix/sysv/linux/tst-pidfd.c
> >> >> > +++ b/sysdeps/unix/sysv/linux/tst-pidfd.c
> >> >> > @@ -95,8 +95,10 @@ do_test (void)
> >> >> >         kernel has pidfd support that we can test.  */
> >> >> >      int r = pidfd_getfd (0, 0, 1);
> >> >> >      TEST_VERIFY_EXIT (r == -1);
> >> >> > -    if (errno == ENOSYS)
> >> >> > -      FAIL_UNSUPPORTED ("kernel does not support pidfd_getfd,
> >> >> > skipping test");
> >> >> > +    if (errno == ENOSYS || errno == EPERM)
> >> >> > +      FAIL_UNSUPPORTED ("kernel does not support pidfd_getfd,"
> >> >> > +			" or we don't have
> >> >> > PTRACE_MODE_ATTACH_REALCREDS"
> >> >> > +			" permissions, skipping test");
> >> >> >    }
> >> >> 
> >> >> This also hints towards a broken seccomp filter.
> >> >
> >> > pidfd_getfd is mentioned (and allowed) by the seccomp filter, but the
> >> > syscall also needs the process to have PTRACE_MODE_ATTACH_REALCREDS
> >> > (which is really PTRACE_MODE_ATTACH | PTRACE_MODE_REALCREDS). Which it
> >> > doesn't have. If the process doesn't then pidfd_getfd is defined as
> >> > failing and setting errno to EPERM.
> >> 
> >> But what does it mean for a process to have PTRACE_MODE_REALCREDS?
> >
> > #define PTRACE_MODE_ATTACH_REALCREDS (PTRACE_MODE_ATTACH | PTRACE_MODE_REALCREDS)
> >
> > PTRACE_MODE_ATTACH_REALCREDS means
> > * PTRACE_MODE_REALCREDS which means cred->{g,u}id of the task are used
> >   for permission checking:
> >
> > So it's a bit nasty here but roughly if the ptracer's real {g,u}id match
> > the target task's (ptracee's) effective, save, and real [g,u]id then
> > you'passed the first stage of permission checking.
> >
> > If ptracer's [g,u]id aren't matching the ptracee's effective, saved, and
> > real [g,u]id the ptracer needs CAP_SYS_PTRACE in the ptracee's userns.
> > That will also get you past the first state of permission checking.
> >
> > If both don't apply the request is denied with -EPERM.
> 
> I think this doesn't apply to the
> 
>   pidfd_getfd (0, 0, 1)
> 
> call because the arguments are invalid and we never get to the kernel
> permission check.  I thought this might be a self-get call, but that's
> clearly not the case (0 does not refer to a process file descriptor).
> 
> So the comment should not mention PTRACE_MODE_ATTACH_REALCREDS at all
> because it's really not relevant to the situation.  This is just another
> case of broken seccomp filters.  So the usual discussion whether we
> should paper over that or not in the test suite applies here as well.

Yeah, that absolutely is a seccomp filter issue. Your example should
give you EINVAL.

Btw, I think you want to use pidfd_get(-EBADF, 0, 0). That isn't as
error prone as passing 1 in the flags argument.

And you should be able to always expect -EBADF for this. I consider this
to be API in all honesty. IOW, if additional arguments are sane and you
pass an invalid fd to any pidfd call then they should give you -EBADF
back.

So based on this assumption you could say that if you don't get -ENOSYS
and don't get -EBADF back then this must be seccomp or something else...

Christian