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

[Richard Guy Briggs <rgb@redhat.com>]

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