linux-kernel - Re: [PATCH v3 1/2] pidfd: add P

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]

Message-ID: <201907290915.8B421306D@keescook>
Date:   Mon, 29 Jul 2019 09:24:38 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Christian Brauner <christian@...uner.io>
Cc:     linux-kernel@...r.kernel.org, oleg@...hat.com,
        torvalds@...ux-foundation.org, arnd@...db.de,
        ebiederm@...ssion.com, joel@...lfernandes.org, tglx@...utronix.de,
        tj@...nel.org, dhowells@...hat.com, jannh@...gle.com,
        luto@...nel.org, akpm@...ux-foundation.org, cyphar@...har.com,
        viro@...iv.linux.org.uk, kernel-team@...roid.com
Subject: Re: [PATCH v3 1/2] pidfd: add P_PIDFD to waitid()

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@...omium.org>

-- 
Kees Cook