Handle running make check in a restricted environment

Handle running make check in a restricted environment

[Mark Wielaard]

Hi,

A couple of tests fail in a restricted environment, like when running
under a systemd service or container with a seccomp filter set. Detect
such environments and mark the test as UNSUPPORTED instead of failing.

The following 4 patches are needed to get glibc-fedora-x86_64 builder,
which runs as a container, to zero FAIL:
https://builder.sourceware.org/buildbot/#/builders/glibc-fedora-x86_64

- time/tst-clock2.c: clock_settime CLOCK_MONOTONIC might return EPERM
- tst-pkey.c: Handle no permission to alloc memory protection keys
- tst-pidfd.c: Test is UNSUPPORTED without
  PTRACE_MODE_ATTACH_REALCREDS
- tst-personality.c: Handle personality failing with errno EPERM

Patches also available at:
https://code.wildebeest.org/git/user/mjw/glibc/log/?h=container-perms

Cheers,

Mark

[PATCH 1/4] time/tst-clock2.c: clock_settime CLOCK_MONOTONIC might return EPERM

[Mark Wielaard]

clock_settime can return errno EPERM if it does not have permission
to set the clock indicated. The test expects setting the monotonic
clock must fail. Which it does. But the errno can be either EINVAL
or EPERM.
---
 time/tst-clock2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/time/tst-clock2.c b/time/tst-clock2.c
index 4c8fb9f247..3f46220832 100644
--- a/time/tst-clock2.c
+++ b/time/tst-clock2.c
@@ -27,10 +27,10 @@ do_test (void)
       puts ("clock_settime(CLOCK_MONOTONIC) did not fail");
       return 1;
     }
-  if (errno != EINVAL)
+  if (errno != EINVAL && errno != EPERM)
     {
-      printf ("clock_settime(CLOCK_MONOTONIC) set errno to %d, expected %d\n",
-	      errno, EINVAL);
+      printf ("clock_settime(CLOCK_MONOTONIC) set errno to %d, expected %d or %d\n",
+	      errno, EINVAL, EPERM);
       return 1;
     }
   return 0;
-- 
2.30.2

Re: [PATCH 1/4] time/tst-clock2.c: clock_settime CLOCK_MONOTONIC might return EPERM

[Florian Weimer]

* Mark Wielaard:

> clock_settime can return errno EPERM if it does not have permission
> to set the clock indicated. The test expects setting the monotonic
> clock must fail. Which it does. But the errno can be either EINVAL
> or EPERM.
> ---
>  time/tst-clock2.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/time/tst-clock2.c b/time/tst-clock2.c
> index 4c8fb9f247..3f46220832 100644
> --- a/time/tst-clock2.c
> +++ b/time/tst-clock2.c
> @@ -27,10 +27,10 @@ do_test (void)
>        puts ("clock_settime(CLOCK_MONOTONIC) did not fail");
>        return 1;
>      }
> -  if (errno != EINVAL)
> +  if (errno != EINVAL && errno != EPERM)
>      {
> -      printf ("clock_settime(CLOCK_MONOTONIC) set errno to %d, expected %d\n",
> -	      errno, EINVAL);
> +      printf ("clock_settime(CLOCK_MONOTONIC) set errno to %d, expected %d or %d\n",
> +	      errno, EINVAL, EPERM);
>        return 1;
>      }
>    return 0;

You could use "errno to %d (%#m)" if you are changing this line anyway.
And isn't the line too long?

Direction looks okay to me.

Re: [PATCH 1/4] time/tst-clock2.c: clock_settime CLOCK_MONOTONIC might return EPERM

[Mark Wielaard]

Hi Florian,

On Sun, 2022-06-26 at 23:15 +0200, Florian Weimer wrote:
> * Mark Wielaard:
> > -      printf ("clock_settime(CLOCK_MONOTONIC) set errno to %d,
> > expected %d\n",
> > -	      errno, EINVAL);
> > +      printf ("clock_settime(CLOCK_MONOTONIC) set errno to %d,
> > expected %d or %d\n",
> > +	      errno, EINVAL, EPERM);
> >        return 1;
> >      }
> >    return 0;
> 
> You could use "errno to %d (%#m)" if you are changing this line
> anyway.
> And isn't the line too long?
> 
> Direction looks okay to me.

Yes, the line is too long. Sorry, my terminal window was too wide. Will
fix and add (%#m) in v2.

Thanks,

Mark

[PATCH 2/4] tst-pkey.c: Handle no permission to alloc memory protection keys

[Mark Wielaard]

pkey_alloc might fail with errno EPERM if there is no permission
to allocate memory protection keys. Use FAIL_UNSUPPORTED in that
case.
---
 sysdeps/unix/sysv/linux/tst-pkey.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sysdeps/unix/sysv/linux/tst-pkey.c b/sysdeps/unix/sysv/linux/tst-pkey.c
index df51f695bc..48a20fa3e0 100644
--- a/sysdeps/unix/sysv/linux/tst-pkey.c
+++ b/sysdeps/unix/sysv/linux/tst-pkey.c
@@ -203,6 +203,9 @@ do_test (void)
         FAIL_UNSUPPORTED
           ("no keys available or kernel does not support memory"
            " protection keys");
+      if (errno == EPERM)
+        FAIL_UNSUPPORTED
+          ("no permission to alloc memory protection keys");
       FAIL_EXIT1 ("pkey_alloc: %m");
     }
   TEST_COMPARE (pkey_get (keys[0]), 0);
-- 
2.30.2

Re: [PATCH 2/4] tst-pkey.c: Handle no permission to alloc memory protection keys

[Florian Weimer]

* Mark Wielaard:

> pkey_alloc might fail with errno EPERM if there is no permission
> to allocate memory protection keys. Use FAIL_UNSUPPORTED in that
> case.
> ---
>  sysdeps/unix/sysv/linux/tst-pkey.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/sysdeps/unix/sysv/linux/tst-pkey.c b/sysdeps/unix/sysv/linux/tst-pkey.c
> index df51f695bc..48a20fa3e0 100644
> --- a/sysdeps/unix/sysv/linux/tst-pkey.c
> +++ b/sysdeps/unix/sysv/linux/tst-pkey.c
> @@ -203,6 +203,9 @@ do_test (void)
>          FAIL_UNSUPPORTED
>            ("no keys available or kernel does not support memory"
>             " protection keys");
> +      if (errno == EPERM)
> +        FAIL_UNSUPPORTED
> +          ("no permission to alloc memory protection keys");
>        FAIL_EXIT1 ("pkey_alloc: %m");
>      }
>    TEST_COMPARE (pkey_get (keys[0]), 0);

It's rather weird to restrict access to a hardening tool.  Is this in
a container, and is the container tool reasonably up to date?  They
should all have switchted to ENOSYS for reducing the system call
profile.

Re: [PATCH 2/4] tst-pkey.c: Handle no permission to alloc memory protection keys

[Florian Weimer]

* Florian Weimer:

> * Mark Wielaard:
>
>> pkey_alloc might fail with errno EPERM if there is no permission
>> to allocate memory protection keys. Use FAIL_UNSUPPORTED in that
>> case.
>> ---
>>  sysdeps/unix/sysv/linux/tst-pkey.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/sysdeps/unix/sysv/linux/tst-pkey.c b/sysdeps/unix/sysv/linux/tst-pkey.c
>> index df51f695bc..48a20fa3e0 100644
>> --- a/sysdeps/unix/sysv/linux/tst-pkey.c
>> +++ b/sysdeps/unix/sysv/linux/tst-pkey.c
>> @@ -203,6 +203,9 @@ do_test (void)
>>          FAIL_UNSUPPORTED
>>            ("no keys available or kernel does not support memory"
>>             " protection keys");
>> +      if (errno == EPERM)
>> +        FAIL_UNSUPPORTED
>> +          ("no permission to alloc memory protection keys");
>>        FAIL_EXIT1 ("pkey_alloc: %m");
>>      }
>>    TEST_COMPARE (pkey_get (keys[0]), 0);
>
> It's rather weird to restrict access to a hardening tool.  Is this in
> a container, and is the container tool reasonably up to date?  They
> should all have switchted to ENOSYS for reducing the system call
> profile.

I was thinking about this bug in particular:

  systemd: Unknown system calls should produce ENOSYS under systemd-nspawn
  <https://bugzilla.redhat.com/show_bug.cgi?id=2040247>

Re: [PATCH 2/4] tst-pkey.c: Handle no permission to alloc memory protection keys

[Mark Wielaard]

Hi Florian,

On Sun, 2022-06-26 at 23:17 +0200, Florian Weimer wrote:
> * Mark Wielaard:
> 
> > pkey_alloc might fail with errno EPERM if there is no permission
> > to allocate memory protection keys. Use FAIL_UNSUPPORTED in that
> > case.
> > ---
> >  sysdeps/unix/sysv/linux/tst-pkey.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/sysdeps/unix/sysv/linux/tst-pkey.c
> > b/sysdeps/unix/sysv/linux/tst-pkey.c
> > index df51f695bc..48a20fa3e0 100644
> > --- a/sysdeps/unix/sysv/linux/tst-pkey.c
> > +++ b/sysdeps/unix/sysv/linux/tst-pkey.c
> > @@ -203,6 +203,9 @@ do_test (void)
> >          FAIL_UNSUPPORTED
> >            ("no keys available or kernel does not support memory"
> >             " protection keys");
> > +      if (errno == EPERM)
> > +        FAIL_UNSUPPORTED
> > +          ("no permission to alloc memory protection keys");
> >        FAIL_EXIT1 ("pkey_alloc: %m");
> >      }
> >    TEST_COMPARE (pkey_get (keys[0]), 0);
> 
> It's rather weird to restrict access to a hardening tool.  Is this in
> a container, and is the container tool reasonably up to date?  They
> should all have switchted to ENOSYS for reducing the system call
> profile.

It is reasonably up to date. This is a container based on Fedora 36
packages running under Fedora CoreOS stable (36.20220605.3.0, Release
Date: Jun 20, 2022) with moby-engine20.10.16.

You are thinking of the fix to set errno to ENOSYS for syscalls that
are "unknown". That is a syscall number higher than any syscall number
mentioned in the seccomp filter. But the pkey calls are simply not
mentioned in the default seccomp filter. And newer syscalls are listed.
So this (EPERM) is the default errno returned in such cases till the
pkey calls are in the default seccomp profile.

https://github.com/moby/moby/issues/43481
https://github.com/moby/moby/issues/42871

In general I think if we detect pkey_alloc fails we should not try to
test and/or FAIL the pkey tests but simply mark it as UNSUPPORTED.
Whether we believe the errno value really should be ENOSYS, ENOSPC or
EINVAL. It isn't really that helpful to explicitly FAIL on EPERM. Sadly
this issue will be with us for a long time.

Cheers,

Mark

Re: [PATCH 2/4] tst-pkey.c: Handle no permission to alloc memory protection keys

[Florian Weimer]

* Mark Wielaard:

> It is reasonably up to date. This is a container based on Fedora 36
> packages running under Fedora CoreOS stable (36.20220605.3.0, Release
> Date: Jun 20, 2022) with moby-engine20.10.16.

Okay.

> You are thinking of the fix to set errno to ENOSYS for syscalls that
> are "unknown". That is a syscall number higher than any syscall number
> mentioned in the seccomp filter. But the pkey calls are simply not
> mentioned in the default seccomp filter. And newer syscalls are listed.
> So this (EPERM) is the default errno returned in such cases till the
> pkey calls are in the default seccomp profile.
>
> https://github.com/moby/moby/issues/43481
> https://github.com/moby/moby/issues/42871

Ah, so Moby is still stuck with the somewhat broken heuristic (where
unrelated syscall list updates break existing syscalls).

Could you switch to podman instead?

> In general I think if we detect pkey_alloc fails we should not try to
> test and/or FAIL the pkey tests but simply mark it as UNSUPPORTED.
> Whether we believe the errno value really should be ENOSYS, ENOSPC or
> EINVAL. It isn't really that helpful to explicitly FAIL on EPERM. Sadly
> this issue will be with us for a long time.

On the other hand, if we just ignore problems like issue 43481 (which is
essentially requesting that PKU is supported in Moby containers by
default), than they might never get fixed.

On the other hand, PKU is a bit of a fringe feature (and it's hard to
change that for core libraries due to the inconstant signal handling
behavior), so maybe we should defang the EPERM error after all.

Thanks,
Florian

Re: [PATCH 2/4] tst-pkey.c: Handle no permission to alloc memory protection keys

[Mark Wielaard]

Hi Florian,

On Mon, Jun 27, 2022 at 01:39:55PM +0200, Florian Weimer wrote:
> * Mark Wielaard:
> > You are thinking of the fix to set errno to ENOSYS for syscalls that
> > are "unknown". That is a syscall number higher than any syscall number
> > mentioned in the seccomp filter. But the pkey calls are simply not
> > mentioned in the default seccomp filter. And newer syscalls are listed.
> > So this (EPERM) is the default errno returned in such cases till the
> > pkey calls are in the default seccomp profile.
> >
> > https://github.com/moby/moby/issues/43481
> > https://github.com/moby/moby/issues/42871
> 
> Ah, so Moby is still stuck with the somewhat broken heuristic (where
> unrelated syscall list updates break existing syscalls).

I think that is an issue for any environment that uses seccomp to
restrict certain syscall features, unless you explicitly list all
syscalls you are left with some default behaviour that might be
surprising in some situations.

> Could you switch to podman instead?

Sadly no. Although podman is fairly compatible with docker on the
command line it has various (minor) incompatibilities when used
through the Docker Engine API (which buildbot does).
https://sourceware.org/pipermail/overseers/2022q2/018406.html

> > In general I think if we detect pkey_alloc fails we should not try to
> > test and/or FAIL the pkey tests but simply mark it as UNSUPPORTED.
> > Whether we believe the errno value really should be ENOSYS, ENOSPC or
> > EINVAL. It isn't really that helpful to explicitly FAIL on EPERM. Sadly
> > this issue will be with us for a long time.
> 
> On the other hand, if we just ignore problems like issue 43481 (which is
> essentially requesting that PKU is supported in Moby containers by
> default), than they might never get fixed.
> 
> On the other hand, PKU is a bit of a fringe feature (and it's hard to
> change that for core libraries due to the inconstant signal handling
> behavior), so maybe we should defang the EPERM error after all.

Right, the EPERM is real, it really doesn't make sense to try to test,
or deliberately fail, the pkey testcase in that case.

Cheers,

Mark

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

[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");
   }
 
   ppid = getpid ();
-- 
2.30.2

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

[Florian Weimer]

* 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.

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

[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 it is confusing I test for errno == ENOSYS || errno == EPERM. I'll
turn that into separate tests and FAIL_UNSUPPORTED messages, so it is
clear which is which in v2.

Thanks,

Mark

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

[Florian Weimer]

* 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?
Don't we have to acquire that in some way, and can we check if *that*
fails?

Thanks,
Florian

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

[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.

The second stage of permission checking is:
(1) If the task isn't dumpable then you'll need CAP_SYS_PTRACE in the
    ptracee's mm user namespace. That userns may differ from the
    ptracee's current userns if the ptracee's userns wasn't privileged
    over the inode of the file it exec'ed and thus could be an ancestor
    userns (see would_dump()).
(2) The LSMs might deny your request.

Christian

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

[Mark Wielaard]

Hi,

On Mon, Jun 27, 2022 at 01:51:41PM +0200, Christian Brauner wrote:
> On Mon, Jun 27, 2022 at 01:14:06PM +0200, Florian Weimer via Libc-alpha wrote:
> > * Mark Wielaard:
> > > 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.
> 
> The second stage of permission checking is:
> (1) If the task isn't dumpable then you'll need CAP_SYS_PTRACE in the
>     ptracee's mm user namespace. That userns may differ from the
>     ptracee's current userns if the ptracee's userns wasn't privileged
>     over the inode of the file it exec'ed and thus could be an ancestor
>     userns (see would_dump()).
> (2) The LSMs might deny your request.

Right, it is pretty difficult to determine why you got an EPERM.  But
failing with EPERM is documented behaviour for pidfd_getfd, so IMHO if
we get EPERM we should simply skip the test whatever the "real" reason
is.

The ptrace manual page lists the whole (6 step) algorithm under
"Ptrace access mode checking":
https://man7.org/linux/man-pages/man2/ptrace.2.html

It also depends on (not) having CAP_SYS_PTRACE and the permission
might be dependend on /proc/sys/kernel/yama/ptrace_scope

Cheers,

Mark

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

[Adhemerval Zanella]

> On 27 Jun 2022, at 11:17, Mark Wielaard <mark@klomp.org> wrote:
> 
> Hi,
> 
> On Mon, Jun 27, 2022 at 01:51:41PM +0200, Christian Brauner wrote:
>> On Mon, Jun 27, 2022 at 01:14:06PM +0200, Florian Weimer via Libc-alpha wrote:
>>> * Mark Wielaard:
>>>> 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.
>> 
>> The second stage of permission checking is:
>> (1) If the task isn't dumpable then you'll need CAP_SYS_PTRACE in the
>>    ptracee's mm user namespace. That userns may differ from the
>>    ptracee's current userns if the ptracee's userns wasn't privileged
>>    over the inode of the file it exec'ed and thus could be an ancestor
>>    userns (see would_dump()).
>> (2) The LSMs might deny your request.
> 
> Right, it is pretty difficult to determine why you got an EPERM.  But
> failing with EPERM is documented behaviour for pidfd_getfd, so IMHO if
> we get EPERM we should simply skip the test whatever the "real" reason
> is.

I agree, this is possible return code for the syscall so we should
handle it on testcase.

> 
> The ptrace manual page lists the whole (6 step) algorithm under
> "Ptrace access mode checking":
> https://man7.org/linux/man-pages/man2/ptrace.2.html
> 
> It also depends on (not) having CAP_SYS_PTRACE and the permission
> might be dependend on /proc/sys/kernel/yama/ptrace_scope
> 
> Cheers,
> 
> Mark
> 

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

[Christian Brauner]

On Mon, Jun 27, 2022 at 04:17:17PM +0200, Mark Wielaard wrote:
> Hi,
> 
> On Mon, Jun 27, 2022 at 01:51:41PM +0200, Christian Brauner wrote:
> > On Mon, Jun 27, 2022 at 01:14:06PM +0200, Florian Weimer via Libc-alpha wrote:
> > > * Mark Wielaard:
> > > > 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.
> > 
> > The second stage of permission checking is:
> > (1) If the task isn't dumpable then you'll need CAP_SYS_PTRACE in the
> >     ptracee's mm user namespace. That userns may differ from the
> >     ptracee's current userns if the ptracee's userns wasn't privileged
> >     over the inode of the file it exec'ed and thus could be an ancestor
> >     userns (see would_dump()).
> > (2) The LSMs might deny your request.
> 
> Right, it is pretty difficult to determine why you got an EPERM.  But
> failing with EPERM is documented behaviour for pidfd_getfd, so IMHO if
> we get EPERM we should simply skip the test whatever the "real" reason
> is.
> 
> The ptrace manual page lists the whole (6 step) algorithm under
> "Ptrace access mode checking":
> https://man7.org/linux/man-pages/man2/ptrace.2.html
> 
> It also depends on (not) having CAP_SYS_PTRACE and the permission
> might be dependend on /proc/sys/kernel/yama/ptrace_scope

Yeah, to be frank, I consider ptrace_may_access() to be the worst piece
of permission checking code in the kernel. It just takes and shakes so
many core concepts that it's hard to understand and difficult to figure
out what exactly caused it to fail.

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

[Florian Weimer]

* 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.

Thanks,
Florian

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

[Adhemerval Zanella]

> On 27 Jun 2022, at 11:42, Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> 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)

The init has PPID 0, which leads to the idea pid argument 0 is valid.
So fdget won’t fail and EPERM will be returned by pidfd_pid.  Maybe
use pidfd_getfd (TYPE_MAXIMUM (pid_t), 0, 1) instead to trigger a
EBADF.

> 
> 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.
> 
> Thanks,
> Florian

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

[Christian Brauner]

On Mon, Jun 27, 2022 at 11:57:02AM -0300, Adhemerval Zanella wrote:
> 
> 
> > On 27 Jun 2022, at 11:42, Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> 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)
> 
> The init has PPID 0, which leads to the idea pid argument 0 is valid.
> So fdget won’t fail and EPERM will be returned by pidfd_pid.  Maybe
> use pidfd_getfd (TYPE_MAXIMUM (pid_t), 0, 1) instead to trigger a
> EBADF.

No, I don't think that's the case.
EPERM can only ever be returned from ptrace_may_access() in
pidfd_getfd() or if there's something like seccomp in the mix.

You should see EINVAL, I think.

SYSCALL_DEFINE3(pidfd_getfd, int, pidfd, int, fd, unsigned int, flags)
                                 0          0                 1
{
	struct pid *pid;
	struct fd f;
	int ret;

	/* flags is currently unused - make sure it's unset */
	if (flags) // that's 1 so you should see -EINVAL
		return -EINVAL;

	f = fdget(pidfd); // That'll get you stdin most likely.
	if (!f.file)
		return -EBADF;

	pid = pidfd_pid(f.file); // That would give you -EBADF see [1]
	if (IS_ERR(pid))
		ret = PTR_ERR(pid);
	else
		ret = pidfd_getfd(pid, fd);

	fdput(f);
	return ret;
}

[1]:

struct pid *pidfd_pid(const struct file *file)
{
	if (file->f_op == &pidfd_fops) // stdin is no pidfd and thus doesn't have pidfd_fops
		return file->private_data;

	return ERR_PTR(-EBADF); // that's what you'd see for stdin
}

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

[Adhemerval Zanella]

> On 27 Jun 2022, at 12:08, Christian Brauner <brauner@kernel.org> wrote:
> 
> On Mon, Jun 27, 2022 at 11:57:02AM -0300, Adhemerval Zanella wrote:
>> 
>> 
>>> On 27 Jun 2022, at 11:42, Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> 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)
>> 
>> The init has PPID 0, which leads to the idea pid argument 0 is valid.
>> So fdget won’t fail and EPERM will be returned by pidfd_pid. Maybe
>> use pidfd_getfd (TYPE_MAXIMUM (pid_t), 0, 1) instead to trigger a
>> EBADF.
> 
> No, I don't think that's the case.
> EPERM can only ever be returned from ptrace_may_access() in
> pidfd_getfd() or if there's something like seccomp in the mix.
> 
> You should see EINVAL, I think.

Yeah, I forgot about the invalid flag argument.  So it does seems a
seccomp issue and I am not sure sure if we should paper over it on glibc
test.

> 
> SYSCALL_DEFINE3(pidfd_getfd, int, pidfd, int, fd, unsigned int, flags)
> 0 0 1
> {
> 	struct pid *pid;
> 	struct fd f;
> 	int ret;
> 
> 	/* flags is currently unused - make sure it's unset */
> 	if (flags) // that's 1 so you should see -EINVAL
> 		return -EINVAL;
> 
> 	f = fdget(pidfd); // That'll get you stdin most likely.
> 	if (!f.file)
> 		return -EBADF;
> 
> 	pid = pidfd_pid(f.file); // That would give you -EBADF see [1]
> 	if (IS_ERR(pid))
> 		ret = PTR_ERR(pid);
> 	else
> 		ret = pidfd_getfd(pid, fd);
> 
> 	fdput(f);
> 	return ret;
> }
> 
> [1]:
> 
> struct pid *pidfd_pid(const struct file *file)
> {
> 	if (file->f_op == &pidfd_fops) // stdin is no pidfd and thus doesn't have pidfd_fops
> 		return file->private_data;
> 
> 	return ERR_PTR(-EBADF); // that's what you'd see for stdin
> }

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

[Mark Wielaard]

Hi Adhemerval,

On Mon, 2022-06-27 at 12:14 -0300, Adhemerval Zanella wrote:
> > You should see EINVAL, I think.
> 
> Yeah, I forgot about the invalid flag argument.  So it does seems a
> seccomp issue and I am not sure sure if we should paper over it on
> glibc test.

I don't think it is papering over the issue. We do detect the EPERM and
record it as an environment that is UNSUPPORTED. Which is imho a better
state than still trying to test and then FAILing the testcase.

Note that in the buildbot the test results and out files are put into
the bunsendb so that you can easily see why a particular test was
UNSUPPORTED (or why it FAILED).

We might not like that there are execution environments where some
syscalls return EPERM, but (sadly) they exist and handling and
recording that in the test (instead of simply failing) seems the best
way to handle that.

It also makes sure we have a zero-FAIL testsuite, which is really
useful.

Cheers,

Mark

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

[Adhemerval Zanella]

> On 27 Jun 2022, at 13:48, Mark Wielaard <mark@klomp.org> wrote:
> 
> Hi Adhemerval,
> 
> On Mon, 2022-06-27 at 12:14 -0300, Adhemerval Zanella wrote:
>>> You should see EINVAL, I think.
>> 
>> Yeah, I forgot about the invalid flag argument.  So it does seems a
>> seccomp issue and I am not sure sure if we should paper over it on
>> glibc test.
> 
> I don't think it is papering over the issue. We do detect the EPERM and
> record it as an environment that is UNSUPPORTED. Which is imho a better
> state than still trying to test and then FAILing the testcase.

At first I though it was the default kernel syscall code path (without
any security hardening or filtering) returning EPERM, but for this 
specific case it seems that an additional module that is return EPERM.

In this specific environment, how can one test that the syscall is not
implemented (ENOSYS) if passing invalid parameters always result in
EPERM? The different semantic whether filtering is present or not is
troublesome and at least on libc.so we do not try fallback. 

although it has caused a lot of issue on older containers (for instance
when glibc started to use clone3), I still think the correct interface 
here is indeed returning ENOSYS and a value different than this is
a potential failure.

What I would expect is to actually handle EPERM on the pidfd_open
at line 118, where it is valid call that might be filtered out.

> 
> Note that in the buildbot the test results and out files are put into
> the bunsendb so that you can easily see why a particular test was
> UNSUPPORTED (or why it FAILED).
> 
> We might not like that there are execution environments where some
> syscalls return EPERM, but (sadly) they exist and handling and
> recording that in the test (instead of simply failing) seems the best
> way to handle that.
> 
> It also makes sure we have a zero-FAIL testsuite, which is really
> useful.
> 
> Cheers,
> 
> Mark

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

[Mark Wielaard]

Hi Adhemerval,

On Mon, 2022-06-27 at 14:03 -0300, Adhemerval Zanella wrote:
> > On 27 Jun 2022, at 13:48, Mark Wielaard <mark@klomp.org> wrote:
> > I don't think it is papering over the issue. We do detect the EPERM and
> > record it as an environment that is UNSUPPORTED. Which is imho a better
> > state than still trying to test and then FAILing the testcase.
> 
> At first I though it was the default kernel syscall code path (without
> any security hardening or filtering) returning EPERM, but for this 
> specific case it seems that an additional module that is return EPERM.
> 
> In this specific environment, how can one test that the syscall is not
> implemented (ENOSYS) if passing invalid parameters always result in
> EPERM? The different semantic whether filtering is present or not is
> troublesome and at least on libc.so we do not try fallback. 
> 
> although it has caused a lot of issue on older containers (for instance
> when glibc started to use clone3), I still think the correct interface 
> here is indeed returning ENOSYS and a value different than this is
> a potential failure.

Completely agreed. If you cannot make a difference between ENOSYS or
EOERM errors it is a pain/impossible to create (or test) the usage of
such a system call. That is why the pathc simply detects the issue,
flags it and doesn't try to run the testcase because that is impossible
to do correctly in that environment.

> What I would expect is to actually handle EPERM on the pidfd_open
> at line 118, where it is valid call that might be filtered out.

Good point. I will add that in a v3.

Thanks,

Mark

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

[Christian Brauner]

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

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

[Adhemerval Zanella]

> On 26 Jun 2022, at 17:59, Mark Wielaard <mark@klomp.org> wrote:
> 
> 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.

From the discussion this is a possible error for the syscall.  However
I think it would be good print different errors for ENOSYS and EPERM.


> ---
> 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");

Keep the ENOSYS error message and add EPERM.

>   }
> 
>   ppid = getpid ();
> -- 
> 2.30.2
> 

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

[Mark Wielaard]

Hi Adhemerval,

On Mon, 2022-06-27 at 11:23 -0300, Adhemerval Zanella wrote:
> I think it would be good print different errors for ENOSYS and EPERM.
> 
> > --- 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");
> 
> Keep the ENOSYS error message and add EPERM.

I posted a v2 that does that:
https://sourceware.org/pipermail/libc-alpha/2022-June/140054.html

Cheers,

Mark

[PATCH 4/4] tst-personality.c: Handle personality failing with errno EPERM

[Mark Wielaard]

When running under a systemd service with LockPersonality set or
in a container with a personality seccomp filter the personality
syscall might fail with errno EPERM for some personality calls
so that the kernel execution domain can not be changed from the
default. Return 77 (to indicate the test is unsupported) in that
case.
---
 sysdeps/unix/sysv/linux/tst-personality.c | 37 ++++++++++++++++++++---
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/tst-personality.c b/sysdeps/unix/sysv/linux/tst-personality.c
index 5bbc3c4d0b..8fbba2b262 100644
--- a/sysdeps/unix/sysv/linux/tst-personality.c
+++ b/sysdeps/unix/sysv/linux/tst-personality.c
@@ -30,13 +30,40 @@ do_test (void)
   errno = 0xdefaced;
   saved_persona = personality (0xffffffff);
 
-  if (personality (test_persona) != saved_persona
-      || personality (0xffffffff) == -1
-      || personality (PER_LINUX) == -1
-      || personality (0xffffffff) != PER_LINUX
-      || 0xdefaced != errno)
+  if (saved_persona == -1 && errno == EPERM)
+    return 77;
+
+  if (personality (test_persona) != saved_persona)
+    {
+      if (errno == EPERM)
+	{
+	  rc = 77;
+	  goto out;
+	}
+      else
+	rc = 1;
+    }
+
+  if (personality (0xffffffff) == -1)
+    rc = 1;
+
+  if (personality (PER_LINUX) == -1)
+    {
+      if (errno == EPERM)
+	{
+	  rc = 77;
+	  goto out;
+	}
+      rc = 1;
+    }
+
+  if (personality (0xffffffff) != PER_LINUX)
+    rc = 1;
+
+  if (0xdefaced != errno)
     rc = 1;
 
+out:
   (void) personality (saved_persona);
   return rc;
 }
-- 
2.30.2