All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] fcntl: remove unused VALID_UPGRADE_FLAGS
@ 2021-04-23 11:10 Christian Brauner
  2021-04-23 11:10 ` [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2() Christian Brauner
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Christian Brauner @ 2021-04-23 11:10 UTC (permalink / raw)
  To: Christoph Hellwig, Aleksa Sarai, Al Viro
  Cc: linux-fsdevel, Richard Guy Briggs, Christian Brauner

From: Christian Brauner <christian.brauner@ubuntu.com>

We currently do not maky use of this feature and should we implement
something like this in the future it's trivial to add it back.

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
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 include/linux/fcntl.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 766fcd973beb..a332e79b3207 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -12,10 +12,6 @@
 	 FASYNC	| O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
 	 O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
 
-/* List of all valid flags for the how->upgrade_mask argument: */
-#define VALID_UPGRADE_FLAGS \
-	(UPGRADE_NOWRITE | UPGRADE_NOREAD)
-
 /* List of all valid flags for the how->resolve argument: */
 #define VALID_RESOLVE_FLAGS \
 	(RESOLVE_NO_XDEV | RESOLVE_NO_MAGICLINKS | RESOLVE_NO_SYMLINKS | \

base-commit: d434405aaab7d0ebc516b68a8fc4100922d7f5ef
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2()
  2021-04-23 11:10 [PATCH 1/3] fcntl: remove unused VALID_UPGRADE_FLAGS Christian Brauner
@ 2021-04-23 11:10 ` Christian Brauner
  2021-04-23 13:50   ` Richard Guy Briggs
                     ` (2 more replies)
  2021-04-23 11:10 ` [PATCH 3/3] test: add openat2() test for invalid upper 32 bit flag value Christian Brauner
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 13+ messages in thread
From: Christian Brauner @ 2021-04-23 11:10 UTC (permalink / raw)
  To: Christoph Hellwig, Aleksa Sarai, Al Viro
  Cc: linux-fsdevel, Richard Guy Briggs, Christian Brauner

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] test: add openat2() test for invalid upper 32 bit flag value
  2021-04-23 11:10 [PATCH 1/3] fcntl: remove unused VALID_UPGRADE_FLAGS Christian Brauner
  2021-04-23 11:10 ` [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2() Christian Brauner
@ 2021-04-23 11:10 ` Christian Brauner
  2021-04-30 15:24   ` Richard Guy Briggs
  2021-04-23 12:36 ` [PATCH 1/3] fcntl: remove unused VALID_UPGRADE_FLAGS Richard Guy Briggs
  2021-04-26 13:33 ` Christoph Hellwig
  3 siblings, 1 reply; 13+ messages in thread
From: Christian Brauner @ 2021-04-23 11:10 UTC (permalink / raw)
  To: Christoph Hellwig, Aleksa Sarai, Al Viro
  Cc: linux-fsdevel, Richard Guy Briggs, Christian Brauner

From: Christian Brauner <christian.brauner@ubuntu.com>

Test that openat2() rejects unknown flags in the upper 32 bit range.

Cc: Richard Guy Briggs <rgb@redhat.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 tools/testing/selftests/openat2/openat2_test.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 381d874cce99..7379e082a994 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -155,7 +155,7 @@ struct flag_test {
 	int err;
 };
 
-#define NUM_OPENAT2_FLAG_TESTS 24
+#define NUM_OPENAT2_FLAG_TESTS 25
 
 void test_openat2_flags(void)
 {
@@ -229,6 +229,11 @@ void test_openat2_flags(void)
 		{ .name = "invalid how.resolve and O_PATH",
 		  .how.flags = O_PATH,
 		  .how.resolve = 0x1337, .err = -EINVAL },
+
+		/* Invalid flags in the upper 32 bits must be rejected. */
+		{ .name = "invalid flags (1 << 63)",
+		  .how.flags = O_RDONLY | (1ULL << 63),
+		  .how.resolve = 0, .err = -EINVAL },
 	};
 
 	BUILD_BUG_ON(ARRAY_LEN(tests) != NUM_OPENAT2_FLAG_TESTS);
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] fcntl: remove unused VALID_UPGRADE_FLAGS
  2021-04-23 11:10 [PATCH 1/3] fcntl: remove unused VALID_UPGRADE_FLAGS Christian Brauner
  2021-04-23 11:10 ` [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2() Christian Brauner
  2021-04-23 11:10 ` [PATCH 3/3] test: add openat2() test for invalid upper 32 bit flag value Christian Brauner
@ 2021-04-23 12:36 ` Richard Guy Briggs
  2021-04-26 13:33 ` Christoph Hellwig
  3 siblings, 0 replies; 13+ messages in thread
From: Richard Guy Briggs @ 2021-04-23 12:36 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Christoph Hellwig, Aleksa Sarai, Al Viro, linux-fsdevel,
	Christian Brauner

On 2021-04-23 13:10, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> 
> We currently do not maky use of this feature and should we implement
> something like this in the future it's trivial to add it back.
> 
> 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
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Suggested-by: Richard Guy Briggs <rgb@redhat.com>
Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  include/linux/fcntl.h | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
> index 766fcd973beb..a332e79b3207 100644
> --- a/include/linux/fcntl.h
> +++ b/include/linux/fcntl.h
> @@ -12,10 +12,6 @@
>  	 FASYNC	| O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
>  	 O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
>  
> -/* List of all valid flags for the how->upgrade_mask argument: */
> -#define VALID_UPGRADE_FLAGS \
> -	(UPGRADE_NOWRITE | UPGRADE_NOREAD)
> -
>  /* List of all valid flags for the how->resolve argument: */
>  #define VALID_RESOLVE_FLAGS \
>  	(RESOLVE_NO_XDEV | RESOLVE_NO_MAGICLINKS | RESOLVE_NO_SYMLINKS | \
> 
> base-commit: d434405aaab7d0ebc516b68a8fc4100922d7f5ef
> -- 
> 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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2()
  2021-04-23 11:10 ` [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2() Christian Brauner
@ 2021-04-23 13:50   ` Richard Guy Briggs
  2021-04-26 13:34   ` Christoph Hellwig
  2021-05-01  0:53   ` Aleksa Sarai
  2 siblings, 0 replies; 13+ messages in thread
From: Richard Guy Briggs @ 2021-04-23 13:50 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Christoph Hellwig, Aleksa Sarai, Al Viro, linux-fsdevel,
	Christian Brauner

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] fcntl: remove unused VALID_UPGRADE_FLAGS
  2021-04-23 11:10 [PATCH 1/3] fcntl: remove unused VALID_UPGRADE_FLAGS Christian Brauner
                   ` (2 preceding siblings ...)
  2021-04-23 12:36 ` [PATCH 1/3] fcntl: remove unused VALID_UPGRADE_FLAGS Richard Guy Briggs
@ 2021-04-26 13:33 ` Christoph Hellwig
  3 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2021-04-26 13:33 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Christoph Hellwig, Aleksa Sarai, Al Viro, linux-fsdevel,
	Richard Guy Briggs, Christian Brauner

On Fri, Apr 23, 2021 at 01:10:35PM +0200, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> 
> We currently do not maky use of this feature and should we implement
> something like this in the future it's trivial to add it back.

Looks like that ->upgrade_mask field never made it into mainline?

Either way removing and unused mask always seems valid, so:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2()
  2021-04-23 11:10 ` [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2() Christian Brauner
  2021-04-23 13:50   ` Richard Guy Briggs
@ 2021-04-26 13:34   ` Christoph Hellwig
  2021-05-01  0:53   ` Aleksa Sarai
  2 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2021-04-26 13:34 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Christoph Hellwig, Aleksa Sarai, Al Viro, linux-fsdevel,
	Richard Guy Briggs, Christian Brauner

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>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] test: add openat2() test for invalid upper 32 bit flag value
  2021-04-23 11:10 ` [PATCH 3/3] test: add openat2() test for invalid upper 32 bit flag value Christian Brauner
@ 2021-04-30 15:24   ` Richard Guy Briggs
  2021-04-30 16:09     ` Christian Brauner
  0 siblings, 1 reply; 13+ messages in thread
From: Richard Guy Briggs @ 2021-04-30 15:24 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Christoph Hellwig, Aleksa Sarai, Al Viro, linux-fsdevel,
	Christian Brauner

On 2021-04-23 13:10, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> 
> Test that openat2() rejects unknown flags in the upper 32 bit range.
> 
> Cc: Richard Guy Briggs <rgb@redhat.com>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  tools/testing/selftests/openat2/openat2_test.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
> index 381d874cce99..7379e082a994 100644
> --- a/tools/testing/selftests/openat2/openat2_test.c
> +++ b/tools/testing/selftests/openat2/openat2_test.c
> @@ -155,7 +155,7 @@ struct flag_test {
>  	int err;
>  };
>  
> -#define NUM_OPENAT2_FLAG_TESTS 24
> +#define NUM_OPENAT2_FLAG_TESTS 25
>  
>  void test_openat2_flags(void)
>  {
> @@ -229,6 +229,11 @@ void test_openat2_flags(void)
>  		{ .name = "invalid how.resolve and O_PATH",
>  		  .how.flags = O_PATH,
>  		  .how.resolve = 0x1337, .err = -EINVAL },
> +
> +		/* Invalid flags in the upper 32 bits must be rejected. */
> +		{ .name = "invalid flags (1 << 63)",
> +		  .how.flags = O_RDONLY | (1ULL << 63),
> +		  .how.resolve = 0, .err = -EINVAL },

This doesn't appear to specifically test for flags over 32 bits.  It
appears to test for flags not included in VALID_OPEN_FLAGS.

"1ULL << 2" would accomplish the same thing, as would "1ULL << 31" due
to the unused flags in the bottom 32 bits.

The test appears to be useful, but misnamed.

If a new flag was added at 1ULL << 33, this test wouldn't notice and it
would still get dropped in build_open_flags() when flags gets assigned
to op->open_flags.

>  	};
>  
>  	BUILD_BUG_ON(ARRAY_LEN(tests) != NUM_OPENAT2_FLAG_TESTS);
> -- 
> 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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] test: add openat2() test for invalid upper 32 bit flag value
  2021-04-30 15:24   ` Richard Guy Briggs
@ 2021-04-30 16:09     ` Christian Brauner
  2021-04-30 16:46       ` Richard Guy Briggs
  0 siblings, 1 reply; 13+ messages in thread
From: Christian Brauner @ 2021-04-30 16:09 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Christian Brauner, Christoph Hellwig, Aleksa Sarai, Al Viro,
	linux-fsdevel

On Fri, Apr 30, 2021 at 11:24:00AM -0400, Richard Guy Briggs wrote:
> On 2021-04-23 13:10, Christian Brauner wrote:
> > From: Christian Brauner <christian.brauner@ubuntu.com>
> > 
> > Test that openat2() rejects unknown flags in the upper 32 bit range.
> > 
> > Cc: Richard Guy Briggs <rgb@redhat.com>
> > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > Cc: linux-fsdevel@vger.kernel.org
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> >  tools/testing/selftests/openat2/openat2_test.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
> > index 381d874cce99..7379e082a994 100644
> > --- a/tools/testing/selftests/openat2/openat2_test.c
> > +++ b/tools/testing/selftests/openat2/openat2_test.c
> > @@ -155,7 +155,7 @@ struct flag_test {
> >  	int err;
> >  };
> >  
> > -#define NUM_OPENAT2_FLAG_TESTS 24
> > +#define NUM_OPENAT2_FLAG_TESTS 25
> >  
> >  void test_openat2_flags(void)
> >  {
> > @@ -229,6 +229,11 @@ void test_openat2_flags(void)
> >  		{ .name = "invalid how.resolve and O_PATH",
> >  		  .how.flags = O_PATH,
> >  		  .how.resolve = 0x1337, .err = -EINVAL },
> > +
> > +		/* Invalid flags in the upper 32 bits must be rejected. */
> > +		{ .name = "invalid flags (1 << 63)",
> > +		  .how.flags = O_RDONLY | (1ULL << 63),
> > +		  .how.resolve = 0, .err = -EINVAL },
> 
> This doesn't appear to specifically test for flags over 32 bits.  It
> appears to test for flags not included in VALID_OPEN_FLAGS.
> 
> "1ULL << 2" would accomplish the same thing, as would "1ULL << 31" due
> to the unused flags in the bottom 32 bits.
> 
> The test appears to be useful, but misnamed.

I mean we can name it test "currently unknown upper bit".

> 
> If a new flag was added at 1ULL << 33, this test wouldn't notice and it

It isn't supposed to notice because it's a known flag. If we add
#define O_FANCY (1ULL << 63)
this test should fail and either would need to be adapted or more likely
be dropped since all bits are taken apparently.

> would still get dropped in build_open_flags() when flags gets assigned
> to op->open_flags.

I didn't intend to add a test whether flags are silently dropped. I
intended to add a test whether any currently unkown bit in the upper 32
bits is loudly rejected instead of silently ignored.

I may misunderstand what kind of test you would like to see here.

Christian

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] test: add openat2() test for invalid upper 32 bit flag value
  2021-04-30 16:09     ` Christian Brauner
@ 2021-04-30 16:46       ` Richard Guy Briggs
  2021-04-30 17:08         ` Christian Brauner
  0 siblings, 1 reply; 13+ messages in thread
From: Richard Guy Briggs @ 2021-04-30 16:46 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Christian Brauner, Christoph Hellwig, Aleksa Sarai, Al Viro,
	linux-fsdevel

On 2021-04-30 18:09, Christian Brauner wrote:
> On Fri, Apr 30, 2021 at 11:24:00AM -0400, Richard Guy Briggs wrote:
> > On 2021-04-23 13:10, Christian Brauner wrote:
> > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > 
> > > Test that openat2() rejects unknown flags in the upper 32 bit range.
> > > 
> > > Cc: Richard Guy Briggs <rgb@redhat.com>
> > > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > > Cc: linux-fsdevel@vger.kernel.org
> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > ---
> > >  tools/testing/selftests/openat2/openat2_test.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
> > > index 381d874cce99..7379e082a994 100644
> > > --- a/tools/testing/selftests/openat2/openat2_test.c
> > > +++ b/tools/testing/selftests/openat2/openat2_test.c
> > > @@ -155,7 +155,7 @@ struct flag_test {
> > >  	int err;
> > >  };
> > >  
> > > -#define NUM_OPENAT2_FLAG_TESTS 24
> > > +#define NUM_OPENAT2_FLAG_TESTS 25
> > >  
> > >  void test_openat2_flags(void)
> > >  {
> > > @@ -229,6 +229,11 @@ void test_openat2_flags(void)
> > >  		{ .name = "invalid how.resolve and O_PATH",
> > >  		  .how.flags = O_PATH,
> > >  		  .how.resolve = 0x1337, .err = -EINVAL },
> > > +
> > > +		/* Invalid flags in the upper 32 bits must be rejected. */
> > > +		{ .name = "invalid flags (1 << 63)",
> > > +		  .how.flags = O_RDONLY | (1ULL << 63),
> > > +		  .how.resolve = 0, .err = -EINVAL },
> > 
> > This doesn't appear to specifically test for flags over 32 bits.  It
> > appears to test for flags not included in VALID_OPEN_FLAGS.
> > 
> > "1ULL << 2" would accomplish the same thing, as would "1ULL << 31" due
> > to the unused flags in the bottom 32 bits.
> > 
> > The test appears to be useful, but misnamed.
> 
> I mean we can name it test "currently unknown upper bit".
> 
> > 
> > If a new flag was added at 1ULL << 33, this test wouldn't notice and it
> 
> It isn't supposed to notice because it's a known flag. If we add
> #define O_FANCY (1ULL << 63)
> this test should fail and either would need to be adapted or more likely
> be dropped since all bits are taken apparently.

If that O_FANCY was added to VALID_OPEN_FLAGS, then this test would fail
to fail since the check in build_open_flags() would have no problem with
it.

> > would still get dropped in build_open_flags() when flags gets assigned
> > to op->open_flags.
> 
> I didn't intend to add a test whether flags are silently dropped. I
> intended to add a test whether any currently unkown bit in the upper 32
> bits is loudly rejected instead of silently ignored.

It appears to be testing for unknown flags regardless of where they are
in the 64 bits, since the incoming flags are tested against
VALID_OPEN_FLAGS.

> I may misunderstand what kind of test you would like to see here.

I think we need two tests:

1) test for unknown flags
2) test for flags that will get dropped in build_open_flags() by the
assignment from (u64) how->flags to (int) op->open_flag.

This second test could be a BUILD_* test.

> Christian

- 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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] test: add openat2() test for invalid upper 32 bit flag value
  2021-04-30 16:46       ` Richard Guy Briggs
@ 2021-04-30 17:08         ` Christian Brauner
  2021-04-30 17:22           ` Richard Guy Briggs
  0 siblings, 1 reply; 13+ messages in thread
From: Christian Brauner @ 2021-04-30 17:08 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Christian Brauner, Christoph Hellwig, Aleksa Sarai, Al Viro,
	linux-fsdevel

On Fri, Apr 30, 2021 at 12:46:25PM -0400, Richard Guy Briggs wrote:
> On 2021-04-30 18:09, Christian Brauner wrote:
> > On Fri, Apr 30, 2021 at 11:24:00AM -0400, Richard Guy Briggs wrote:
> > > On 2021-04-23 13:10, Christian Brauner wrote:
> > > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > > 
> > > > Test that openat2() rejects unknown flags in the upper 32 bit range.
> > > > 
> > > > Cc: Richard Guy Briggs <rgb@redhat.com>
> > > > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > > > Cc: linux-fsdevel@vger.kernel.org
> > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > ---
> > > >  tools/testing/selftests/openat2/openat2_test.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
> > > > index 381d874cce99..7379e082a994 100644
> > > > --- a/tools/testing/selftests/openat2/openat2_test.c
> > > > +++ b/tools/testing/selftests/openat2/openat2_test.c
> > > > @@ -155,7 +155,7 @@ struct flag_test {
> > > >  	int err;
> > > >  };
> > > >  
> > > > -#define NUM_OPENAT2_FLAG_TESTS 24
> > > > +#define NUM_OPENAT2_FLAG_TESTS 25
> > > >  
> > > >  void test_openat2_flags(void)
> > > >  {
> > > > @@ -229,6 +229,11 @@ void test_openat2_flags(void)
> > > >  		{ .name = "invalid how.resolve and O_PATH",
> > > >  		  .how.flags = O_PATH,
> > > >  		  .how.resolve = 0x1337, .err = -EINVAL },
> > > > +
> > > > +		/* Invalid flags in the upper 32 bits must be rejected. */
> > > > +		{ .name = "invalid flags (1 << 63)",
> > > > +		  .how.flags = O_RDONLY | (1ULL << 63),
> > > > +		  .how.resolve = 0, .err = -EINVAL },
> > > 
> > > This doesn't appear to specifically test for flags over 32 bits.  It
> > > appears to test for flags not included in VALID_OPEN_FLAGS.
> > > 
> > > "1ULL << 2" would accomplish the same thing, as would "1ULL << 31" due
> > > to the unused flags in the bottom 32 bits.
> > > 
> > > The test appears to be useful, but misnamed.
> > 
> > I mean we can name it test "currently unknown upper bit".
> > 
> > > 
> > > If a new flag was added at 1ULL << 33, this test wouldn't notice and it
> > 
> > It isn't supposed to notice because it's a known flag. If we add
> > #define O_FANCY (1ULL << 63)
> > this test should fail and either would need to be adapted or more likely
> > be dropped since all bits are taken apparently.
> 
> If that O_FANCY was added to VALID_OPEN_FLAGS, then this test would fail
> to fail since the check in build_open_flags() would have no problem with
> it.

Right but that's perfectly fine and just means you need to update the
test. That's why this is 1ULL << 63 which moves this way into the
future.

> 
> > > would still get dropped in build_open_flags() when flags gets assigned
> > > to op->open_flags.
> > 
> > I didn't intend to add a test whether flags are silently dropped. I
> > intended to add a test whether any currently unkown bit in the upper 32
> > bits is loudly rejected instead of silently ignored.
> 
> It appears to be testing for unknown flags regardless of where they are
> in the 64 bits, since the incoming flags are tested against
> VALID_OPEN_FLAGS.

I fail to see the fundamental issue (even with the name) but I happily
rename it to "currently unknown bit in upper 32 bits rejected" to
indicate that.

> 
> > I may misunderstand what kind of test you would like to see here.
> 
> I think we need two tests:
> 
> 1) test for unknown flags
> 2) test for flags that will get dropped in build_open_flags() by the
> assignment from (u64) how->flags to (int) op->open_flag.
> 
> This second test could be a BUILD_* test.

Yes, that makes sense. Thank you.
I think that can be a build test based on VALID_OPEN_FLAGS. I think the
assumption that any new flag needs to be added to this define is
perfectly fine?

Christian

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] test: add openat2() test for invalid upper 32 bit flag value
  2021-04-30 17:08         ` Christian Brauner
@ 2021-04-30 17:22           ` Richard Guy Briggs
  0 siblings, 0 replies; 13+ messages in thread
From: Richard Guy Briggs @ 2021-04-30 17:22 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Christian Brauner, Christoph Hellwig, Aleksa Sarai, Al Viro,
	linux-fsdevel

On 2021-04-30 19:08, Christian Brauner wrote:
> On Fri, Apr 30, 2021 at 12:46:25PM -0400, Richard Guy Briggs wrote:
> > On 2021-04-30 18:09, Christian Brauner wrote:
> > > On Fri, Apr 30, 2021 at 11:24:00AM -0400, Richard Guy Briggs wrote:
> > > > On 2021-04-23 13:10, Christian Brauner wrote:
> > > > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > 
> > > > > Test that openat2() rejects unknown flags in the upper 32 bit range.
> > > > > 
> > > > > Cc: Richard Guy Briggs <rgb@redhat.com>
> > > > > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > > > > Cc: linux-fsdevel@vger.kernel.org
> > > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > ---
> > > > >  tools/testing/selftests/openat2/openat2_test.c | 7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
> > > > > index 381d874cce99..7379e082a994 100644
> > > > > --- a/tools/testing/selftests/openat2/openat2_test.c
> > > > > +++ b/tools/testing/selftests/openat2/openat2_test.c
> > > > > @@ -155,7 +155,7 @@ struct flag_test {
> > > > >  	int err;
> > > > >  };
> > > > >  
> > > > > -#define NUM_OPENAT2_FLAG_TESTS 24
> > > > > +#define NUM_OPENAT2_FLAG_TESTS 25
> > > > >  
> > > > >  void test_openat2_flags(void)
> > > > >  {
> > > > > @@ -229,6 +229,11 @@ void test_openat2_flags(void)
> > > > >  		{ .name = "invalid how.resolve and O_PATH",
> > > > >  		  .how.flags = O_PATH,
> > > > >  		  .how.resolve = 0x1337, .err = -EINVAL },
> > > > > +
> > > > > +		/* Invalid flags in the upper 32 bits must be rejected. */
> > > > > +		{ .name = "invalid flags (1 << 63)",
> > > > > +		  .how.flags = O_RDONLY | (1ULL << 63),
> > > > > +		  .how.resolve = 0, .err = -EINVAL },
> > > > 
> > > > This doesn't appear to specifically test for flags over 32 bits.  It
> > > > appears to test for flags not included in VALID_OPEN_FLAGS.
> > > > 
> > > > "1ULL << 2" would accomplish the same thing, as would "1ULL << 31" due
> > > > to the unused flags in the bottom 32 bits.
> > > > 
> > > > The test appears to be useful, but misnamed.
> > > 
> > > I mean we can name it test "currently unknown upper bit".
> > > 
> > > > 
> > > > If a new flag was added at 1ULL << 33, this test wouldn't notice and it
> > > 
> > > It isn't supposed to notice because it's a known flag. If we add
> > > #define O_FANCY (1ULL << 63)
> > > this test should fail and either would need to be adapted or more likely
> > > be dropped since all bits are taken apparently.
> > 
> > If that O_FANCY was added to VALID_OPEN_FLAGS, then this test would fail
> > to fail since the check in build_open_flags() would have no problem with
> > it.
> 
> Right but that's perfectly fine and just means you need to update the
> test. That's why this is 1ULL << 63 which moves this way into the
> future.

It is temping to change the test to (-1ULL & ~VALID_OPEN_FLAGS) to catch
any outside what has been expressly defined.  (Not certain about syntax...)

> > > > would still get dropped in build_open_flags() when flags gets assigned
> > > > to op->open_flags.
> > > 
> > > I didn't intend to add a test whether flags are silently dropped. I
> > > intended to add a test whether any currently unkown bit in the upper 32
> > > bits is loudly rejected instead of silently ignored.
> > 
> > It appears to be testing for unknown flags regardless of where they are
> > in the 64 bits, since the incoming flags are tested against
> > VALID_OPEN_FLAGS.
> 
> I fail to see the fundamental issue (even with the name) but I happily
> rename it to "currently unknown bit in upper 32 bits rejected" to
> indicate that.

How about "currently unknown bit rejected"?

> > > I may misunderstand what kind of test you would like to see here.
> > 
> > I think we need two tests:
> > 
> > 1) test for unknown flags
> > 2) test for flags that will get dropped in build_open_flags() by the
> > assignment from (u64) how->flags to (int) op->open_flag.
> > 
> > This second test could be a BUILD_* test.
> 
> Yes, that makes sense. Thank you.
> I think that can be a build test based on VALID_OPEN_FLAGS. I think the
> assumption that any new flag needs to be added to this define is
> perfectly fine?

I don't see how else we can do this other than to throw a runtime error
which will happen anyways the first time userspace tries to use a new
flag that isn't included in VALID_OPEN_FLAGS in the first place, so I
think this is a safe assumption.

> Christian

- 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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2()
  2021-04-23 11:10 ` [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2() Christian Brauner
  2021-04-23 13:50   ` Richard Guy Briggs
  2021-04-26 13:34   ` Christoph Hellwig
@ 2021-05-01  0:53   ` Aleksa Sarai
  2 siblings, 0 replies; 13+ messages in thread
From: Aleksa Sarai @ 2021-05-01  0:53 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Christoph Hellwig, Al Viro, linux-fsdevel, Richard Guy Briggs,
	Christian Brauner

[-- Attachment #1: Type: text/plain, Size: 1979 bytes --]

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>

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-05-01  0:54 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-23 11:10 [PATCH 1/3] fcntl: remove unused VALID_UPGRADE_FLAGS Christian Brauner
2021-04-23 11:10 ` [PATCH 2/3] open: don't silently ignore unknown O-flags in openat2() Christian Brauner
2021-04-23 13:50   ` Richard Guy Briggs
2021-04-26 13:34   ` Christoph Hellwig
2021-05-01  0:53   ` Aleksa Sarai
2021-04-23 11:10 ` [PATCH 3/3] test: add openat2() test for invalid upper 32 bit flag value Christian Brauner
2021-04-30 15:24   ` Richard Guy Briggs
2021-04-30 16:09     ` Christian Brauner
2021-04-30 16:46       ` Richard Guy Briggs
2021-04-30 17:08         ` Christian Brauner
2021-04-30 17:22           ` Richard Guy Briggs
2021-04-23 12:36 ` [PATCH 1/3] fcntl: remove unused VALID_UPGRADE_FLAGS Richard Guy Briggs
2021-04-26 13:33 ` Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.