| Message ID | 20231120193914.441117-3-mic@digikod.net (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Extend Landlock test to improve rule's coverage | expand |
On Mon, Nov 20, 2023 at 08:39:14PM +0100, Mickaël Salaün wrote: > Add two tests to make sure that we cannot add a rule to a ruleset if the > rule's access rights that are not handled by the ruleset: > * fs: layout1.rule_with_unhandled_access > * net: mini.rule_with_unhandled_access > > Cc: Günther Noack <gnoack@google.com> > Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > Signed-off-by: Mickaël Salaün <mic@digikod.net> > --- > tools/testing/selftests/landlock/fs_test.c | 35 +++++++++++++++++++++ > tools/testing/selftests/landlock/net_test.c | 33 +++++++++++++++++++ > 2 files changed, 68 insertions(+) > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > index d77155d75de5..8cabcbe3554e 100644 > --- a/tools/testing/selftests/landlock/fs_test.c > +++ b/tools/testing/selftests/landlock/fs_test.c > @@ -596,6 +596,41 @@ TEST_F_FORK(layout1, file_and_dir_access_rights) > ASSERT_EQ(0, close(ruleset_fd)); > } > > +TEST_F_FORK(layout1, rule_with_unhandled_access) > +{ > + struct landlock_ruleset_attr ruleset_attr = { > + /* First bit */ > + .handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE, Optional nit: If you want to spell out that this is 1, you could as well add an assertion for that. Doesn't even need to be a static_assert, it's just a test after all. Or maybe even put a literal 1 here instead. :) > + }; > + struct landlock_path_beneath_attr path_beneath = {}; > + int ruleset_fd; > + __u64 access; > + > + ruleset_fd = > + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); > + ASSERT_LE(0, ruleset_fd); > + > + path_beneath.parent_fd = open(file1_s1d2, O_PATH | O_CLOEXEC); > + ASSERT_LE(0, path_beneath.parent_fd); > + > + for (access = 1; access > 0; access <<= 1) { > + int err; > + > + path_beneath.allowed_access = access; > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, > + &path_beneath, 0); > + if (access == ruleset_attr.handled_access_fs) { > + EXPECT_EQ(0, err); > + } else { > + EXPECT_EQ(-1, err); > + EXPECT_EQ(EINVAL, errno); > + } > + } > + > + EXPECT_EQ(0, close(path_beneath.parent_fd)); > + EXPECT_EQ(0, close(ruleset_fd)); > +} > + > TEST_F_FORK(layout0, unknown_access_rights) > { > __u64 access_mask; > diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c > index 9356f5800e31..aec01917abd5 100644 > --- a/tools/testing/selftests/landlock/net_test.c > +++ b/tools/testing/selftests/landlock/net_test.c > @@ -1262,6 +1262,39 @@ TEST_F(mini, network_access_rights) > EXPECT_EQ(0, close(ruleset_fd)); > } > > +TEST_F(mini, rule_with_unhandled_access) > +{ > + struct landlock_ruleset_attr ruleset_attr = { > + /* First bit */ > + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP, Ditto. > + }; > + struct landlock_net_port_attr net_port = { > + .port = sock_port_start, > + }; > + 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; > + > + net_port.allowed_access = access; > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT, > + &net_port, 0); > + if (access == ruleset_attr.handled_access_net) { > + EXPECT_EQ(0, err); > + } else { > + EXPECT_EQ(-1, err); > + EXPECT_EQ(EINVAL, errno); > + } > + } > + > + EXPECT_EQ(0, close(ruleset_fd)); > +} > + > /* Checks invalid attribute, out of landlock network access range. */ > TEST_F(mini, unknown_access_rights) > { > -- > 2.42.1 > Reviewed-by: Günther Noack <gnoack@google.com> Thanks for the tests! —Günther
11/20/2023 10:39 PM, Mickaël Salaün пишет: > Add two tests to make sure that we cannot add a rule to a ruleset if the > rule's access rights that are not handled by the ruleset: > * fs: layout1.rule_with_unhandled_access > * net: mini.rule_with_unhandled_access > > Cc: Günther Noack <gnoack@google.com> > Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > Signed-off-by: Mickaël Salaün <mic@digikod.net> > --- > tools/testing/selftests/landlock/fs_test.c | 35 +++++++++++++++++++++ > tools/testing/selftests/landlock/net_test.c | 33 +++++++++++++++++++ > 2 files changed, 68 insertions(+) > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > index d77155d75de5..8cabcbe3554e 100644 > --- a/tools/testing/selftests/landlock/fs_test.c > +++ b/tools/testing/selftests/landlock/fs_test.c > @@ -596,6 +596,41 @@ TEST_F_FORK(layout1, file_and_dir_access_rights) > ASSERT_EQ(0, close(ruleset_fd)); > } > > +TEST_F_FORK(layout1, rule_with_unhandled_access) > +{ > + struct landlock_ruleset_attr ruleset_attr = { > + /* First bit */ > + .handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE, > + }; > + struct landlock_path_beneath_attr path_beneath = {}; > + int ruleset_fd; > + __u64 access; > + > + ruleset_fd = > + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); > + ASSERT_LE(0, ruleset_fd); > + > + path_beneath.parent_fd = open(file1_s1d2, O_PATH | O_CLOEXEC); > + ASSERT_LE(0, path_beneath.parent_fd); > + > + for (access = 1; access > 0; access <<= 1) { > + int err; > + > + path_beneath.allowed_access = access; > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, > + &path_beneath, 0); > + if (access == ruleset_attr.handled_access_fs) { > + EXPECT_EQ(0, err); > + } else { > + EXPECT_EQ(-1, err); > + EXPECT_EQ(EINVAL, errno); > + } > + } > + > + EXPECT_EQ(0, close(path_beneath.parent_fd)); > + EXPECT_EQ(0, close(ruleset_fd)); > +} > + > TEST_F_FORK(layout0, unknown_access_rights) > { > __u64 access_mask; > diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c > index 9356f5800e31..aec01917abd5 100644 > --- a/tools/testing/selftests/landlock/net_test.c > +++ b/tools/testing/selftests/landlock/net_test.c > @@ -1262,6 +1262,39 @@ TEST_F(mini, network_access_rights) > EXPECT_EQ(0, close(ruleset_fd)); > } > > +TEST_F(mini, rule_with_unhandled_access) > +{ > + struct landlock_ruleset_attr ruleset_attr = { > + /* First bit */ > + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP, > + }; > + struct landlock_net_port_attr net_port = { > + .port = sock_port_start, > + }; > + 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; > + > + net_port.allowed_access = access; > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT, > + &net_port, 0); > + if (access == ruleset_attr.handled_access_net) { > + EXPECT_EQ(0, err); > + } else { > + EXPECT_EQ(-1, err); > + EXPECT_EQ(EINVAL, errno); > + } > + } We have such kind of check in TEST_f(mini, inval). Can you please explain why we need additional one here? > + > + EXPECT_EQ(0, close(ruleset_fd)); > +} > + > /* Checks invalid attribute, out of landlock network access range. */ > TEST_F(mini, unknown_access_rights) > {
On Fri, Nov 24, 2023 at 06:12:52PM +0100, Günther Noack wrote: > On Mon, Nov 20, 2023 at 08:39:14PM +0100, Mickaël Salaün wrote: > > Add two tests to make sure that we cannot add a rule to a ruleset if the > > rule's access rights that are not handled by the ruleset: > > * fs: layout1.rule_with_unhandled_access > > * net: mini.rule_with_unhandled_access > > > > Cc: Günther Noack <gnoack@google.com> > > Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > --- > > tools/testing/selftests/landlock/fs_test.c | 35 +++++++++++++++++++++ > > tools/testing/selftests/landlock/net_test.c | 33 +++++++++++++++++++ > > 2 files changed, 68 insertions(+) > > > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > > index d77155d75de5..8cabcbe3554e 100644 > > --- a/tools/testing/selftests/landlock/fs_test.c > > +++ b/tools/testing/selftests/landlock/fs_test.c > > @@ -596,6 +596,41 @@ TEST_F_FORK(layout1, file_and_dir_access_rights) > > ASSERT_EQ(0, close(ruleset_fd)); > > } > > > > +TEST_F_FORK(layout1, rule_with_unhandled_access) > > +{ > > + struct landlock_ruleset_attr ruleset_attr = { > > + /* First bit */ > > + .handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE, > > Optional nit: If you want to spell out that this is 1, you could as well add an > assertion for that. Doesn't even need to be a static_assert, it's just a test > after all. Or maybe even put a literal 1 here instead. :) I'll remove this comment because it is not relevant anymore because the loop check the related value. > > > + }; > > + struct landlock_path_beneath_attr path_beneath = {}; > > + int ruleset_fd; > > + __u64 access; > > + > > + ruleset_fd = > > + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); > > + ASSERT_LE(0, ruleset_fd); > > + > > + path_beneath.parent_fd = open(file1_s1d2, O_PATH | O_CLOEXEC); > > + ASSERT_LE(0, path_beneath.parent_fd); > > + > > + for (access = 1; access > 0; access <<= 1) { > > + int err; > > + > > + path_beneath.allowed_access = access; > > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, > > + &path_beneath, 0); > > + if (access == ruleset_attr.handled_access_fs) { > > + EXPECT_EQ(0, err); > > + } else { > > + EXPECT_EQ(-1, err); > > + EXPECT_EQ(EINVAL, errno); > > + } > > + } > > + > > + EXPECT_EQ(0, close(path_beneath.parent_fd)); > > + EXPECT_EQ(0, close(ruleset_fd)); > > +} > > + > > TEST_F_FORK(layout0, unknown_access_rights) > > { > > __u64 access_mask; > > diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c > > index 9356f5800e31..aec01917abd5 100644 > > --- a/tools/testing/selftests/landlock/net_test.c > > +++ b/tools/testing/selftests/landlock/net_test.c > > @@ -1262,6 +1262,39 @@ TEST_F(mini, network_access_rights) > > EXPECT_EQ(0, close(ruleset_fd)); > > } > > > > +TEST_F(mini, rule_with_unhandled_access) > > +{ > > + struct landlock_ruleset_attr ruleset_attr = { > > + /* First bit */ > > + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP, > > Ditto. > > > + }; > > + struct landlock_net_port_attr net_port = { > > + .port = sock_port_start, > > + }; > > + 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; > > + > > + net_port.allowed_access = access; > > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT, > > + &net_port, 0); > > + if (access == ruleset_attr.handled_access_net) { > > + EXPECT_EQ(0, err); > > + } else { > > + EXPECT_EQ(-1, err); > > + EXPECT_EQ(EINVAL, errno); > > + } > > + } > > + > > + EXPECT_EQ(0, close(ruleset_fd)); > > +} > > + > > /* Checks invalid attribute, out of landlock network access range. */ > > TEST_F(mini, unknown_access_rights) > > { > > -- > > 2.42.1 > > > > Reviewed-by: Günther Noack <gnoack@google.com> > > Thanks for the tests! > —Günther >
On Mon, Nov 27, 2023 at 11:04:02AM +0300, Konstantin Meskhidze (A) wrote: > > > 11/20/2023 10:39 PM, Mickaël Salaün пишет: > > Add two tests to make sure that we cannot add a rule to a ruleset if the > > rule's access rights that are not handled by the ruleset: > > * fs: layout1.rule_with_unhandled_access > > * net: mini.rule_with_unhandled_access > > > > Cc: Günther Noack <gnoack@google.com> > > Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > --- > > tools/testing/selftests/landlock/fs_test.c | 35 +++++++++++++++++++++ > > tools/testing/selftests/landlock/net_test.c | 33 +++++++++++++++++++ > > 2 files changed, 68 insertions(+) > > > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > > index d77155d75de5..8cabcbe3554e 100644 > > --- a/tools/testing/selftests/landlock/fs_test.c > > +++ b/tools/testing/selftests/landlock/fs_test.c > > @@ -596,6 +596,41 @@ TEST_F_FORK(layout1, file_and_dir_access_rights) > > ASSERT_EQ(0, close(ruleset_fd)); > > } > > +TEST_F_FORK(layout1, rule_with_unhandled_access) > > +{ > > + struct landlock_ruleset_attr ruleset_attr = { > > + /* First bit */ > > + .handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE, > > + }; > > + struct landlock_path_beneath_attr path_beneath = {}; > > + int ruleset_fd; > > + __u64 access; > > + > > + ruleset_fd = > > + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); > > + ASSERT_LE(0, ruleset_fd); > > + > > + path_beneath.parent_fd = open(file1_s1d2, O_PATH | O_CLOEXEC); > > + ASSERT_LE(0, path_beneath.parent_fd); > > + > > + for (access = 1; access > 0; access <<= 1) { > > + int err; > > + > > + path_beneath.allowed_access = access; > > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, > > + &path_beneath, 0); > > + if (access == ruleset_attr.handled_access_fs) { > > + EXPECT_EQ(0, err); > > + } else { > > + EXPECT_EQ(-1, err); > > + EXPECT_EQ(EINVAL, errno); > > + } > > + } > > + > > + EXPECT_EQ(0, close(path_beneath.parent_fd)); > > + EXPECT_EQ(0, close(ruleset_fd)); > > +} > > + > > TEST_F_FORK(layout0, unknown_access_rights) > > { > > __u64 access_mask; > > diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c > > index 9356f5800e31..aec01917abd5 100644 > > --- a/tools/testing/selftests/landlock/net_test.c > > +++ b/tools/testing/selftests/landlock/net_test.c > > @@ -1262,6 +1262,39 @@ TEST_F(mini, network_access_rights) > > EXPECT_EQ(0, close(ruleset_fd)); > > } > > +TEST_F(mini, rule_with_unhandled_access) > > +{ > > + struct landlock_ruleset_attr ruleset_attr = { > > + /* First bit */ > > + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP, > > + }; > > + struct landlock_net_port_attr net_port = { > > + .port = sock_port_start, > > + }; > > + 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; > > + > > + net_port.allowed_access = access; > > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT, > > + &net_port, 0); > > + if (access == ruleset_attr.handled_access_net) { > > + EXPECT_EQ(0, err); > > + } else { > > + EXPECT_EQ(-1, err); > > + EXPECT_EQ(EINVAL, errno); > > + } > > + } > > We have such kind of check in TEST_f(mini, inval). Can you please explain > why we need additional one here? This doesn't test the same thing. This new test checks that a only known access rights can be added, which should be a subset of the handled access rights. This is mostly useful to check consistency with the synthetic/private access rights Günther is working on. > > + > > + EXPECT_EQ(0, close(ruleset_fd)); > > +} > > + > > /* Checks invalid attribute, out of landlock network access range. */ > > TEST_F(mini, unknown_access_rights) > > { >
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index d77155d75de5..8cabcbe3554e 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -596,6 +596,41 @@ TEST_F_FORK(layout1, file_and_dir_access_rights) ASSERT_EQ(0, close(ruleset_fd)); } +TEST_F_FORK(layout1, rule_with_unhandled_access) +{ + struct landlock_ruleset_attr ruleset_attr = { + /* First bit */ + .handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE, + }; + struct landlock_path_beneath_attr path_beneath = {}; + int ruleset_fd; + __u64 access; + + ruleset_fd = + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); + ASSERT_LE(0, ruleset_fd); + + path_beneath.parent_fd = open(file1_s1d2, O_PATH | O_CLOEXEC); + ASSERT_LE(0, path_beneath.parent_fd); + + for (access = 1; access > 0; access <<= 1) { + int err; + + path_beneath.allowed_access = access; + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, + &path_beneath, 0); + if (access == ruleset_attr.handled_access_fs) { + EXPECT_EQ(0, err); + } else { + EXPECT_EQ(-1, err); + EXPECT_EQ(EINVAL, errno); + } + } + + EXPECT_EQ(0, close(path_beneath.parent_fd)); + EXPECT_EQ(0, close(ruleset_fd)); +} + TEST_F_FORK(layout0, unknown_access_rights) { __u64 access_mask; diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c index 9356f5800e31..aec01917abd5 100644 --- a/tools/testing/selftests/landlock/net_test.c +++ b/tools/testing/selftests/landlock/net_test.c @@ -1262,6 +1262,39 @@ TEST_F(mini, network_access_rights) EXPECT_EQ(0, close(ruleset_fd)); } +TEST_F(mini, rule_with_unhandled_access) +{ + struct landlock_ruleset_attr ruleset_attr = { + /* First bit */ + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP, + }; + struct landlock_net_port_attr net_port = { + .port = sock_port_start, + }; + 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; + + net_port.allowed_access = access; + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT, + &net_port, 0); + if (access == ruleset_attr.handled_access_net) { + EXPECT_EQ(0, err); + } else { + EXPECT_EQ(-1, err); + EXPECT_EQ(EINVAL, errno); + } + } + + EXPECT_EQ(0, close(ruleset_fd)); +} + /* Checks invalid attribute, out of landlock network access range. */ TEST_F(mini, unknown_access_rights) {
Add two tests to make sure that we cannot add a rule to a ruleset if the rule's access rights that are not handled by the ruleset: * fs: layout1.rule_with_unhandled_access * net: mini.rule_with_unhandled_access Cc: Günther Noack <gnoack@google.com> Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> Signed-off-by: Mickaël Salaün <mic@digikod.net> --- tools/testing/selftests/landlock/fs_test.c | 35 +++++++++++++++++++++ tools/testing/selftests/landlock/net_test.c | 33 +++++++++++++++++++ 2 files changed, 68 insertions(+)