linux-kernel - Re: [PATCH] pid: allow pidfds for reaped tasks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 07 Aug 2023 11:12:11 +0200
From:   "David Rheinsberg" <david@...dahead.eu>
To:     "Alexander Mikhalitsyn" <alexander@...alicyn.com>
Cc:     linux-kernel@...r.kernel.org,
        "Christian Brauner" <brauner@...nel.org>,
        "Jan Kara" <jack@...e.cz>, "Kees Cook" <keescook@...omium.org>,
        "Luca Boccassi" <bluca@...ian.org>
Subject: Re: [PATCH] pid: allow pidfds for reaped tasks

Hi

On Mon, Aug 7, 2023, at 11:01 AM, Alexander Mikhalitsyn wrote:
> On Mon, Aug 7, 2023 at 10:52 AM David Rheinsberg <david@...dahead.eu> wrote:
[...]
>>  int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret)
>>  {
>> -       if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
>> +       if (!pid)
>> +               return -EINVAL;
>> +
>> +       /*
>> +        * Non thread-group leaders cannot have pidfds, but we allow them for
>> +        * reaped thread-group leaders.
>> +        */
>> +       if (pid_has_task(pid, PIDTYPE_PID) && !pid_has_task(pid, PIDTYPE_TGID))
>>                 return -EINVAL;
>
> Hi David!
>
> As far as I understand, __unhash_process is always called with a
> tasklist_lock held for writing.
> Don't we need to take tasklist_lock for reading here to guarantee
> consistency between
> pid_has_task(pid, PIDTYPE_PID) and pid_has_task(pid, PIDTYPE_TGID)
> return values?

You mean PIDTYPE_TGID being cleared before PIDTYPE_PID (at least from the perspective of the unlocked reader)? I don't think it is a compatibility issue, because the same issue existed before the patch. But it might indeed be required to avoid spurious EINVAL _while_ the target process is reaped.

It would be unfortunate if we need that. Because it is really not required for AF_UNIX or fanotify (they guarantee that they always deal with TGIDs). So maybe the correct call is to just drop pidfd_prepare() and always use __pidfd_prepare()? So far the safety-measures of pidfd_prepare() introduced two races I already mentioned in the commit-message. So maybe it is just better to document that the caller of __pidfd_prepare() needs to ensure the source is/was a TGID?

Thanks
David