diff mbox series

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

Message ID 20220626205915.33201-3-mark@klomp.org
State Superseded
Headers
Series [1/4] time/tst-clock2.c: clock_settime CLOCK_MONOTONIC might return EPERM |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Mark Wielaard June 26, 2022, 8:59 p.m. UTC 
  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(+)

Comments

Florian Weimer June 26, 2022, 9:17 p.m. UTC | #1 
* 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.
Florian Weimer June 26, 2022, 9:40 p.m. UTC | #2 
* 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>
Mark Wielaard June 27, 2022, 9:50 a.m. UTC | #3 
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
Florian Weimer June 27, 2022, 11:39 a.m. UTC | #4 
* 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
Mark Wielaard June 27, 2022, 2:08 p.m. UTC | #5 
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
diff mbox series

Patch

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