Re: [RFC PATCH v3 06/19] selftests/landlock: Test adding a rule for unhandled access

["Günther Noack" <gnoack@google.com>]

On Wed, Sep 11, 2024 at 11:19:48AM +0300, Mikhail Ivanov wrote:
> On 9/10/2024 12:22 PM, Günther Noack wrote:
> > Hi!
> > 
> > On Wed, Sep 04, 2024 at 06:48:11PM +0800, Mikhail Ivanov wrote:
> > > Add test that validates behaviour of Landlock after rule with
> > > unhandled access is added.
> > > 
> > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> > > ---
> > > Changes since v2:
> > > * Replaces EXPECT_EQ with ASSERT_EQ for close().
> > > * Refactors commit title and message.
> > > 
> > > Changes since v1:
> > > * Refactors commit message.
> > > ---
> > >   .../testing/selftests/landlock/socket_test.c  | 33 +++++++++++++++++++
> > >   1 file changed, 33 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> > > index 811bdaa95a7a..d2fedfca7193 100644
> > > --- a/tools/testing/selftests/landlock/socket_test.c
> > > +++ b/tools/testing/selftests/landlock/socket_test.c
> > > @@ -351,4 +351,37 @@ TEST_F(protocol, rule_with_unknown_access)
> > >   	ASSERT_EQ(0, close(ruleset_fd));
> > >   }
> > > +TEST_F(protocol, rule_with_unhandled_access)
> > > +{
> > > +	struct landlock_ruleset_attr ruleset_attr = {
> > > +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> > > +	};
> > > +	struct landlock_socket_attr protocol = {
> > > +		.family = self->prot.family,
> > > +		.type = self->prot.type,
> > > +	};
> > > +	int ruleset_fd;
> > > +	__u64 access;
> > > +
> > > +	ruleset_fd =
> > > +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> > > +	ASSERT_LE(0, ruleset_fd);
> > > +
> > > +	for (access = 1; access > 0; access <<= 1) {
> > > +		int err;
> > > +
> > > +		protocol.allowed_access = access;
> > > +		err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> > > +					&protocol, 0);
> > > +		if (access == ruleset_attr.handled_access_socket) {
> > > +			EXPECT_EQ(0, err);
> > > +		} else {
> > > +			EXPECT_EQ(-1, err);
> > > +			EXPECT_EQ(EINVAL, errno);
> > > +		}
> > > +	}
> > > +
> > > +	ASSERT_EQ(0, close(ruleset_fd));
> > > +}
> > > +
> > 
> > I should probably have noticed this on the first review round; you are not
> > actually exercising any scenario here where a rule with unhandled access is
> > added.
> > 
> > To clarify, the notion of an access right being "unhandled" means that the
> > access right was not listed at ruleset creation time in the ruleset_attr's
> > .handled_access_* field where it would have belonged.  If that is the case,
> > adding a ruleset with that access right is going to be denied.
> > 
> > As an example:
> > If the ruleset only handles LANDLOCK_ACCESS_FS_WRITE_FILE and nothing else,
> > then, if the test tries to insert a rule for LANDLOCK_ACCESS_SOCKET_CREATE,
> > that call is supposed to fail -- because the "socket creation" access right is
> > not handled.
> 
> This test was added to exercise adding a rule with future possible
> "unhandled" access rights of "socket" type, but since this patch
> implements only one, this test is really meaningless. Thank you for
> this note!
> 
> > 
> > IMHO the test would become more reasonable if it was more clearly "handling"
> > something entirely unrelated at ruleset creation time, e.g. one of the file
> > system access rights.  (And we could do the same for the "net" and "fs" tests as
> > well.)
> > 
> > Your test is a copy of the same test for the "net" rights, which in turn is a
> > copy of teh same test for the "fs" rights.  When the "fs" test was written, the
> > "fs" access rights were the only ones that could be used at all to create a
> > ruleset, but this is not true any more.
> 
> Good idea! Can I implement such test in the current patchset?

Yes, I think it would be a good idea.

I would, in fact, recommend to turn the rule_with_unhandled_access test into that test.

The test traces its roots clearly to

  TEST_F(mini, rule_with_unhandled_access)  from net_test.c

and to

  TEST_F_FORK(layout1, rule_with_unhandled_access)  from fs_test.c


and I think all three variants would better be advised to create a ruleset with

struct landlock_ruleset_attr ruleset_attr = {
	.handled_access_something_entirely_different = LANDLOCK_ACCESS_WHATEVER,
}

and then check their corresponding fs, net and socket access rights using a
landlock_add_rule() call for the access rights that belong to the respective
module, so that it exercises the scenario where userspace attempts to use the
access right in a rule, but the surrounding ruleset did not restrict the same
access right (it was "unhandled").

In spirit, it would be nicest if we could create a ruleset where nothing at all
is handled, but I believe in that case, the landlock_create_ruleset() call would
already fail.

—Günther

P.S.: I am starting to grow a bit uncomfortable with the amount of duplicated
test code that we start having across the different types of access rights.  If
you see a way to keep this more in check, while still keeping the tests
expressive and not over-frameworking them, let's try to move in that direction
if we can. :)