Re: [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2()

From: Richard Guy Briggs <rgb@redhat.com>
To: Christian Brauner <brauner@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>, Aleksa Sarai <cyphar@cyphar.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org,
	Christian Brauner <christian.brauner@ubuntu.com>
Subject: Re: [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2()
Date: Fri, 23 Apr 2021 09:50:42 -0400	[thread overview]
Message-ID: <20210423135042.GM3141668@madcap2.tricolour.ca> (raw)
In-Reply-To: <20210423111037.3590242-2-brauner@kernel.org>

On 2021-04-23 13:10, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> 
> The new openat2() syscall verifies that no unknown O-flag values are
> set and returns an error to userspace if they are while the older open
> syscalls like open() and openat2() simply ignore unknown flag values:
> 
>   #define O_FLAG_CURRENTLY_INVALID (1 << 31)
>   struct open_how how = {
>           .flags = O_RDONLY | O_FLAG_CURRENTLY_INVALID,
>           .resolve = 0,
>   };
> 
>   /* fails */
>   fd = openat2(-EBADF, "/dev/null", &how, sizeof(how));
> 
>   /* succeeds */
>   fd = openat(-EBADF, "/dev/null", O_RDONLY | O_FLAG_CURRENTLY_INVALID);
> 
> However, openat2() silently truncates the upper 32 bits meaning:
> 
>   #define O_FLAG_CURRENTLY_INVALID_LOWER32 (1 << 31)
>   #define O_FLAG_CURRENTLY_INVALID_UPPER32 (1 << 40)
> 
>   struct open_how how_lowe32 = {
>           .flags = O_RDONLY | O_FLAG_CURRENTLY_INVALID_LOWE32,
>           .resolve = 0,
>   };
> 
>   struct open_how how_upper32 = {
>           .flags = O_RDONLY | O_FLAG_CURRENTLY_INVALID_LOWE32,
>           .resolve = 0,
>   };
> 
>   /* fails */
>   fd = openat2(-EBADF, "/dev/null", &how_lower32, sizeof(how_lower32));
> 
>   /* succeeds */
>   fd = openat2(-EBADF, "/dev/null", &how_upper32, sizeof(how_upper32));
> 
> That seems like a bug. Fix it by preventing the truncation in
> build_open_flags().
> 
> There's a snafu here though stripping FMODE_* directly from flags would
> cause the upper 32 bits to be truncated as well due to integer promotion
> rules since FMODE_* is unsigned int, O_* are signed ints (yuck).
> 
> This change shouldn't regress old open syscalls since they silently
> truncate any unknown values.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> Reported-by: Richard Guy Briggs <rgb@redhat.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  fs/open.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index e53af13b5835..96644aa325eb 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1002,12 +1002,17 @@ inline struct open_how build_open_how(int flags, umode_t mode)
>  
>  inline int build_open_flags(const struct open_how *how, struct open_flags *op)
>  {
> -	int flags = how->flags;
> +	u64 flags = how->flags;
> +	u64 strip = FMODE_NONOTIFY | O_CLOEXEC;
>  	int lookup_flags = 0;
>  	int acc_mode = ACC_MODE(flags);
>  
> -	/* Must never be set by userspace */
> -	flags &= ~(FMODE_NONOTIFY | O_CLOEXEC);
> +	/*
> +	 * Strip flags that either shouldn't be set by userspace like
> +	 * FMODE_NONOTIFY or that aren't relevant in determining struct
> +	 * open_flags like O_CLOEXEC.
> +	 */
> +	flags &= ~strip;

Would it not be simpler to only change flags' type (and elaborated
comment) and leave the original strip or will that run afoul of FMODE_*
type clamping to u32?

To guard against this assignment of u64 flags to op->open_flags losing
info in the future further down in this function, it would be necessary
to add something like the following that you suggested to
include/linux/fcntl.h following the definition of VALID_OPEN_FLAGS:

	BUILD_BUG_ON_MSG(upper_32_bits(VALID_OPEN_FLAGS), "will be ignored by open_flags assignment in build_open_flags()");

A similar check could be added for O_ACCMODE for 32 bits in general, and
for 8 bits for Tomoyo.

>  	/*
>  	 * Older syscalls implicitly clear all of the invalid flags or argument
> -- 
> 2.27.0

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635