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@xxxxxxxxxx>
> > > > >
> > > > > Test that openat2() rejects unknown flags in the upper 32 bit range.
> > > > >
> > > > > Cc: Richard Guy Briggs <rgb@xxxxxxxxxx>
> > > > > Cc: Aleksa Sarai <cyphar@xxxxxxxxxx>
> > > > > Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> > > > > Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxxx>
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
