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@xxxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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. :)
