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 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
> }