[PATCH v1 0/2] Extend Landlock test to improve rule's coverage

[PATCH v1 0/2] Extend Landlock test to improve rule's coverage

[Mickaël Salaün]

Hi,

These are new tests to give more backward compatibility guarantees about
rule's access rights. This might be useful for changes happening in the
access right handling, especially with synthetic access rights:
https://lore.kernel.org/r/20231117154920.1706371-3-gnoack@google.com

Regards,

Mickaël Salaün (2):
  selftests/landlock: Add tests to check undefined rule's access rights
  selftests/landlock: Add tests to check unhandled rule's access rights

 tools/testing/selftests/landlock/fs_test.c  | 52 +++++++++++++++++++--
 tools/testing/selftests/landlock/net_test.c | 50 +++++++++++++++++---
 2 files changed, 90 insertions(+), 12 deletions(-)


base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
-- 
2.42.1

[PATCH v1 1/2] selftests/landlock: Add tests to check undefined rule's access rights

[Mickaël Salaün]

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;
+		}
 
 		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));
-- 
2.42.1

[PATCH v1 2/2] selftests/landlock: Add tests to check unhandled rule's access rights

[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);
+		}
+	}
+
+	EXPECT_EQ(0, close(ruleset_fd));
+}
+
 /* Checks invalid attribute, out of landlock network access range. */
 TEST_F(mini, unknown_access_rights)
 {
-- 
2.42.1

Re: [PATCH v1 1/2] selftests/landlock: Add tests to check undefined rule's access rights

[Günther Noack]

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

Re: [PATCH v1 2/2] selftests/landlock: Add tests to check unhandled rule's access rights

[Günther Noack]

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

Re: [PATCH v1 2/2] selftests/landlock: Add tests to check unhandled rule's access rights

[Konstantin Meskhidze (A)]

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)
>   {

Re: [PATCH v1 1/2] selftests/landlock: Add tests to check undefined rule's access rights

[Mickaël Salaün]

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
> 

Re: [PATCH v1 2/2] selftests/landlock: Add tests to check unhandled rule's access rights

[Mickaël Salaün]

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
> 

Re: [PATCH v1 2/2] selftests/landlock: Add tests to check unhandled rule's access rights

[Mickaël Salaün]

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)
> >   {
>