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

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

> 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