Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 29 Jul 2019 09:24:38 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Christian Brauner <christian@brauner.io>
Cc:     linux-kernel@vger.kernel.org, oleg@redhat.com,
        torvalds@linux-foundation.org, arnd@arndb.de,
        ebiederm@xmission.com, joel@joelfernandes.org, tglx@linutronix.de,
        tj@kernel.org, dhowells@redhat.com, jannh@google.com,
        luto@kernel.org, akpm@linux-foundation.org, cyphar@cyphar.com,
        viro@zeniv.linux.org.uk, kernel-team@android.com
Subject: Re: [PATCH v3 1/2] pidfd: add P_PIDFD to waitid()
Message-ID: <201907290915.8B421306D@keescook>
References: <20190727222229.6516-1-christian@brauner.io>
 <20190727222229.6516-2-christian@brauner.io>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190727222229.6516-2-christian@brauner.io>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Jul 28, 2019 at 12:22:29AM +0200, Christian Brauner wrote:
> diff --git a/kernel/exit.c b/kernel/exit.c
> index a75b6a7f458a..64bb6893a37d 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1552,6 +1552,23 @@ static long do_wait(struct wait_opts *wo)
>  	return retval;
>  }
>  
> +static struct pid *pidfd_get_pid(unsigned int fd)
> +{
> +	struct fd f;
> +	struct pid *pid;
> +
> +	f = fdget(fd);
> +	if (!f.file)
> +		return ERR_PTR(-EBADF);
> +
> +	pid = pidfd_pid(f.file);
> +	if (!IS_ERR(pid))
> +		get_pid(pid);
> +
> +	fdput(f);
> +	return pid;
> +}
> +
>  static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
>  			  int options, struct rusage *ru)
>  {
> @@ -1574,19 +1591,29 @@ static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
>  		type = PIDTYPE_PID;
>  		if (upid <= 0)
>  			return -EINVAL;
> +
> +		pid = find_get_pid(upid);
>  		break;
>  	case P_PGID:
>  		type = PIDTYPE_PGID;
>  		if (upid <= 0)
>  			return -EINVAL;
> +
> +		pid = find_get_pid(upid);
> +		break;
> +	case P_PIDFD:
> +		type = PIDTYPE_PID;
> +		if (upid < 0)
> +			return -EINVAL;
> +
> +		pid = pidfd_get_pid(upid);
> +		if (IS_ERR(pid))
> +			return PTR_ERR(pid);

I spent some time convincing myself that this early bail out was
correct. It seems this path is only reachable in the EBADF case, so that
makes sense. The other failure modes in this switch all give a NULL pid
so that the final do_wait() returns ECHILD. So, as long as that's
intentional (which I think it is), this all looks fine. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook