| Message ID | 20231120193914.441117-2-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:13PM +0100, Mickaël Salaün wrote: > Extend two tests to make sure that we cannot add a rule with access > rights that are undefined: > * fs: layout1.file_and_dir_access_rights > * net: mini.network_access_rights > > The checks test all 64 bits access right values until it overflows. > > Replace one ASSERT with EXPECT in layout1.file_and_dir_access_rights . > > 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 | 17 ++++++++++++----- > tools/testing/selftests/landlock/net_test.c | 17 ++++++++++------- > 2 files changed, 22 insertions(+), 12 deletions(-) > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > index 18e1f86a6234..d77155d75de5 100644 > --- a/tools/testing/selftests/landlock/fs_test.c > +++ b/tools/testing/selftests/landlock/fs_test.c > @@ -548,7 +548,6 @@ TEST_F_FORK(layout1, inval) > TEST_F_FORK(layout1, file_and_dir_access_rights) > { > __u64 access; > - int err; > struct landlock_path_beneath_attr path_beneath_file = {}, > path_beneath_dir = {}; > struct landlock_ruleset_attr ruleset_attr = { > @@ -568,11 +567,19 @@ TEST_F_FORK(layout1, file_and_dir_access_rights) > open(dir_s1d2, O_PATH | O_DIRECTORY | O_CLOEXEC); > ASSERT_LE(0, path_beneath_dir.parent_fd); > > - for (access = 1; access <= ACCESS_LAST; access <<= 1) { > + for (access = 1; access > 0; access <<= 1) { > + int err; > + > path_beneath_dir.allowed_access = access; > - ASSERT_EQ(0, landlock_add_rule(ruleset_fd, > - LANDLOCK_RULE_PATH_BENEATH, > - &path_beneath_dir, 0)); > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, > + &path_beneath_dir, 0); > + if (access <= ACCESS_LAST) { > + EXPECT_EQ(0, err); > + } else { > + EXPECT_EQ(-1, err); > + EXPECT_EQ(EINVAL, errno); > + continue; > + } Style question: why not have two loops next to each other? You could keep the old loop from 1 to ACCESS_LAST and then have a separate one from ACCESS_LAST+1 onwards. Then you would not need to put logic inside the loop; it might reduce nesting a bit, and each loop individually might be slightly easier to grasp. I was initially a bit confused why the other landlock_add_rule() call for the directory doesn't need the same change. That is clear to me after looking at the code a few seconds longer, but it might be slightly simpler with two separate loops. But this is a minor nit. Reviewed-by: Günther Noack <gnoack@google.com> Thanks! —Günther
On Fri, Nov 24, 2023 at 06:07:05PM +0100, Günther Noack wrote: > On Mon, Nov 20, 2023 at 08:39:13PM +0100, Mickaël Salaün wrote: > > Extend two tests to make sure that we cannot add a rule with access > > rights that are undefined: > > * fs: layout1.file_and_dir_access_rights > > * net: mini.network_access_rights > > > > The checks test all 64 bits access right values until it overflows. > > > > Replace one ASSERT with EXPECT in layout1.file_and_dir_access_rights . > > > > 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 | 17 ++++++++++++----- > > tools/testing/selftests/landlock/net_test.c | 17 ++++++++++------- > > 2 files changed, 22 insertions(+), 12 deletions(-) > > > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > > index 18e1f86a6234..d77155d75de5 100644 > > --- a/tools/testing/selftests/landlock/fs_test.c > > +++ b/tools/testing/selftests/landlock/fs_test.c > > @@ -548,7 +548,6 @@ TEST_F_FORK(layout1, inval) > > TEST_F_FORK(layout1, file_and_dir_access_rights) > > { > > __u64 access; > > - int err; > > struct landlock_path_beneath_attr path_beneath_file = {}, > > path_beneath_dir = {}; > > struct landlock_ruleset_attr ruleset_attr = { > > @@ -568,11 +567,19 @@ TEST_F_FORK(layout1, file_and_dir_access_rights) > > open(dir_s1d2, O_PATH | O_DIRECTORY | O_CLOEXEC); > > ASSERT_LE(0, path_beneath_dir.parent_fd); > > > > - for (access = 1; access <= ACCESS_LAST; access <<= 1) { > > + for (access = 1; access > 0; access <<= 1) { > > + int err; > > + > > path_beneath_dir.allowed_access = access; > > - ASSERT_EQ(0, landlock_add_rule(ruleset_fd, > > - LANDLOCK_RULE_PATH_BENEATH, > > - &path_beneath_dir, 0)); > > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, > > + &path_beneath_dir, 0); > > + if (access <= ACCESS_LAST) { > > + EXPECT_EQ(0, err); > > + } else { > > + EXPECT_EQ(-1, err); > > + EXPECT_EQ(EINVAL, errno); > > + continue; > > + } > > Style question: why not have two loops next to each other? You could keep the > old loop from 1 to ACCESS_LAST and then have a separate one from ACCESS_LAST+1 > onwards. Then you would not need to put logic inside the loop; it might reduce > nesting a bit, and each loop individually might be slightly easier to grasp. > > I was initially a bit confused why the other landlock_add_rule() call for the > directory doesn't need the same change. That is clear to me after looking at the > code a few seconds longer, but it might be slightly simpler with two separate > loops. Indeed, I'll send a v2. > > But this is a minor nit. > > Reviewed-by: Günther Noack <gnoack@google.com> > > Thanks! > —Günther >
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 18e1f86a6234..d77155d75de5 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -548,7 +548,6 @@ TEST_F_FORK(layout1, inval) TEST_F_FORK(layout1, file_and_dir_access_rights) { __u64 access; - int err; struct landlock_path_beneath_attr path_beneath_file = {}, path_beneath_dir = {}; struct landlock_ruleset_attr ruleset_attr = { @@ -568,11 +567,19 @@ TEST_F_FORK(layout1, file_and_dir_access_rights) open(dir_s1d2, O_PATH | O_DIRECTORY | O_CLOEXEC); ASSERT_LE(0, path_beneath_dir.parent_fd); - for (access = 1; access <= ACCESS_LAST; access <<= 1) { + for (access = 1; access > 0; access <<= 1) { + int err; + path_beneath_dir.allowed_access = access; - ASSERT_EQ(0, landlock_add_rule(ruleset_fd, - LANDLOCK_RULE_PATH_BENEATH, - &path_beneath_dir, 0)); + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, + &path_beneath_dir, 0); + if (access <= ACCESS_LAST) { + EXPECT_EQ(0, err); + } else { + EXPECT_EQ(-1, err); + EXPECT_EQ(EINVAL, errno); + continue; + } path_beneath_file.allowed_access = access; err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c index 929e21c4db05..9356f5800e31 100644 --- a/tools/testing/selftests/landlock/net_test.c +++ b/tools/testing/selftests/landlock/net_test.c @@ -1246,14 +1246,17 @@ TEST_F(mini, network_access_rights) landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); ASSERT_LE(0, ruleset_fd); - for (access = 1; access <= ACCESS_LAST; access <<= 1) { + for (access = 1; access > 0; access <<= 1) { + int err; + net_port.allowed_access = access; - EXPECT_EQ(0, - landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT, - &net_port, 0)) - { - TH_LOG("Failed to add rule with access 0x%llx: %s", - access, strerror(errno)); + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT, + &net_port, 0); + if (access <= ACCESS_LAST) { + EXPECT_EQ(0, err); + } else { + EXPECT_EQ(-1, err); + EXPECT_EQ(EINVAL, errno); } } EXPECT_EQ(0, close(ruleset_fd));
Extend two tests to make sure that we cannot add a rule with access rights that are undefined: * fs: layout1.file_and_dir_access_rights * net: mini.network_access_rights The checks test all 64 bits access right values until it overflows. Replace one ASSERT with EXPECT in layout1.file_and_dir_access_rights . 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 | 17 ++++++++++++----- tools/testing/selftests/landlock/net_test.c | 17 ++++++++++------- 2 files changed, 22 insertions(+), 12 deletions(-)