diff mbox series

[3/3] test: add openat2() test for invalid upper 32 bit flag value

Message ID 20210423111037.3590242-3-brauner@kernel.org (mailing list archive)
State New
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>

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(-)

Comments

Richard Guy Briggs April 30, 2021, 3:24 p.m. UTC | #1
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
Christian Brauner April 30, 2021, 4:09 p.m. UTC | #2
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
Richard Guy Briggs April 30, 2021, 4:46 p.m. UTC | #3
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
Christian Brauner April 30, 2021, 5:08 p.m. UTC | #4
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
Richard Guy Briggs April 30, 2021, 5:22 p.m. UTC | #5
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
diff mbox series

Patch

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);