diff mbox series

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

Message ID 20210423111037.3590242-2-brauner@kernel.org (mailing list archive)
State New, archived
Headers show
Series [1/3] fcntl: remove unused VALID_UPGRADE_FLAGS | expand

Commit Message

Christian Brauner April 23, 2021, 11:10 a.m. UTC 
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(-)

Comments

Richard Guy Briggs April 23, 2021, 1:50 p.m. UTC | #1 
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
Christoph Hellwig April 26, 2021, 1:34 p.m. UTC | #2 
On Fri, Apr 23, 2021 at 01:10:36PM +0200, Christian Brauner wrote:
> 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.

So, this is a change in behavior for openat.  But given how new it is
and there are not flags defined yet in the truncated range I think
we should be fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Aleksa Sarai May 1, 2021, 12:53 a.m. UTC | #3 
On 2021-04-23, Christian Brauner <brauner@kernel.org> wrote:
> 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.

Yeah, oops on my part (I was always worried I'd missed something with
making everything -EINVAL).

Reviewed-by: Aleksa Sarai <cyphar@cyphar.com>
diff mbox series

Patch

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;
 
 	/*
 	 * Older syscalls implicitly clear all of the invalid flags or argument