linux-kernel - Re: [PATCH 2/3] docs, parallelism: Do not leak blocking mode to writer

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: <041953ef-0b6c-4ea8-8734-aa1e6703f9f8@rasmusvillemoes.dk>
Date:   Thu, 21 Nov 2019 08:41:01 +0100
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Kees Cook <keescook@...omium.org>, Jonathan Corbet <corbet@....net>
Cc:     linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] docs, parallelism: Do not leak blocking mode to
 writer

On 21/11/2019 01.03, Kees Cook wrote:
> Setting non-blocking via a local copy of the jobserver file descriptor
> is safer than just assuming the writer on the original fd is prepared
> for it to be non-blocking.

This is a bit inaccurate. The fd referring to the write side of the pipe
is always blocking - it has to be, due to the protocol requiring you to
write back the tokens you've read, so you can't just drop a token on the
floor. But it's also rather moot, since the pipe will never hold
anywhere near 4096 bytes, let alone a (linux) pipe's default capacity of
64K.

But what we cannot do is change the mode of the open file description to
non-blocking for the read side, in case the parent make (or some sibling
process that has also inherited the same "struct file") expects it to be
blocking.

> Suggested-by: Rasmus Villemoes <linux@...musvillemoes.dk>
> Link: https://lore.kernel.org/lkml/44c01043-ab24-b4de-6544-e8efd153e27a@rasmusvillemoes.dk
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
>  scripts/jobserver-count | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/jobserver-count b/scripts/jobserver-count
> index 6e15b38df3d0..a68a04ad304f 100755
> --- a/scripts/jobserver-count
> +++ b/scripts/jobserver-count
> @@ -12,12 +12,6 @@ default="1"
>  if len(sys.argv) > 1:
>  	default=sys.argv[1]
>  
> -# Set non-blocking for a given file descriptor.
> -def nonblock(fd):
> -	flags = fcntl.fcntl(fd, fcntl.F_GETFL)
> -	fcntl.fcntl(fd, fcntl.F_SETFL, flags | os.O_NONBLOCK)
> -	return fd
> -
>  # Extract and prepare jobserver file descriptors from envirnoment.
>  try:
>  	# Fetch the make environment options.
> @@ -31,8 +25,13 @@ try:
>  	# Parse out R,W file descriptor numbers and set them nonblocking.
>  	fds = opts[0].split("=", 1)[1]
>  	reader, writer = [int(x) for x in fds.split(",", 1)]
> -	reader = nonblock(reader)
> -except (KeyError, IndexError, ValueError, IOError):
> +	# Open a private copy of reader to avoid setting nonblocking
> +	# on an unexpecting writer.

s/writer/reader/

> +	reader = os.open("/proc/self/fd/%d" % (reader), os.O_RDONLY)
> +	flags = fcntl.fcntl(reader, fcntl.F_GETFL)
> +	fcntl.fcntl(reader, fcntl.F_SETFL, flags | os.O_NONBLOCK)

I think you can just specify O_NONBLOCK in the open() call so you avoid
those two fcntls.

Rasmus