linux-kernel - Re: [PATCH v2 1/4] pidfd: support PIDFD_NONBLOCK in pidfd

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:   Thu, 3 Sep 2020 16:58:09 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Christian Brauner <christian.brauner@...ntu.com>
Cc:     linux-kernel@...r.kernel.org,
        Christian Brauner <christian@...uner.io>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Kees Cook <keescook@...omium.org>,
        Sargun Dhillon <sargun@...gun.me>,
        Aleksa Sarai <cyphar@...har.com>,
        linux-kselftest@...r.kernel.org,
        Josh Triplett <josh@...htriplett.org>,
        Jens Axboe <axboe@...nel.dk>, linux-api@...r.kernel.org
Subject: Re: [PATCH v2 1/4] pidfd: support PIDFD_NONBLOCK in pidfd_open()

Christian, off-topic question...

On 09/02, Christian Brauner wrote:
>
> -static int pidfd_create(struct pid *pid)
> +static int pidfd_create(struct pid *pid, unsigned int flags)
>  {
>  	int fd;
>
>  	fd = anon_inode_getfd("[pidfd]", &pidfd_fops, get_pid(pid),
> -			      O_RDWR | O_CLOEXEC);
> +			      flags | O_RDWR | O_CLOEXEC);

I just noticed this comment above pidfd_create:

	 * Note, that this function can only be called after the fd table has
	 * been unshared to avoid leaking the pidfd to the new process.

what does it mean?

Of course, if fd table is shared then pidfd can "leak" to another process,
but this is true for any file and sys_pidfd_open() doesn't do any check?



In fact I think this helper buys nothing but adds the unnecessary get/put_pid,
we can kill it and change pidfd_open() to do

	SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
	{
		int fd;
		struct pid *p;

		if (flags & ~PIDFD_NONBLOCK)
			return -EINVAL;

		if (pid <= 0)
			return -EINVAL;

		p = find_get_pid(pid);
		if (!p)
			return -ESRCH;

		fd = -EINVAL;
		if (pid_has_task(p, PIDTYPE_TGID)) {
			fd = anon_inode_getfd("[pidfd]", &pidfd_fops, pid,
						flags | O_RDWR | O_CLOEXEC);
		}
		if (fd < 0)
			put_pid(p);
		return fd;
	}

but this is cosmetic and off-topic too.

Oleg.