10/20/2023 6:41 PM, Mickaël Salaün пишет:

On Fri, Oct 20, 2023 at 02:58:31PM +0300, Konstantin Meskhidze (A) wrote:

quoted

10/20/2023 12:49 PM, Mickaël Salaün пишет:

quoted

On Fri, Oct 20, 2023 at 07:08:33AM +0300, Konstantin Meskhidze (A) wrote:

quoted

10/18/2023 3:29 PM, Mickaël Salaün пишет:

quoted

On Mon, Oct 16, 2023 at 09:50:26AM +0800, Konstantin Meskhidze wrote:

quoted

quoted

quoted

quoted

quoted

diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 4c209acee01e..1fe4298ff4a7 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c

@@ -36,6 +36,11 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
 	refcount_set(&new_ruleset->usage, 1);
 	mutex_init(&new_ruleset->lock);
 	new_ruleset->root_inode = RB_ROOT;
+
+#if IS_ENABLED(CONFIG_INET)
+	new_ruleset->root_net_port = RB_ROOT;
+#endif /* IS_ENABLED(CONFIG_INET) */
+
 	new_ruleset->num_layers = num_layers;
 	/*
 	 * hierarchy = NULL

@@ -46,16 +51,21 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
 }

quoted

quoted

 struct landlock_ruleset *

-landlock_create_ruleset(const access_mask_t fs_access_mask)
+landlock_create_ruleset(const access_mask_t fs_access_mask,
+			const access_mask_t net_access_mask)
 {
 	struct landlock_ruleset *new_ruleset;

quoted

quoted

 	/* Informs about useless ruleset. */

-	if (!fs_access_mask)
+	if (!fs_access_mask && !net_access_mask)
 		return ERR_PTR(-ENOMSG);
 	new_ruleset = create_ruleset(1);
-	if (!IS_ERR(new_ruleset))
+	if (IS_ERR(new_ruleset))
+		return new_ruleset;
+	if (fs_access_mask)
 		landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
+	if (net_access_mask)
+		landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
This is good, but it is not tested: we need to add a test that

both

quoted

handle FS and net restrictions. You can add one in net.c, just handling
LANDLOCK_ACCESS_FS_READ_DIR and LANDLOCK_ACCESS_NET_BIND_TCP, add one
rule with path_beneath (e.g. /dev) and another with net_port, and check
that open("/") is denied, open("/dev") is allowed, and and only the
allowed port is allowed with bind(). This test should be simple and can
only check against an IPv4 socket, i.e. using ipv4_tcp fixture, just
after port_endianness. fcntl.h should then be included by net.c

  Ok.

quoted

quoted

I guess that was the purpose of layout1.with_net (in fs_test.c)

but it

  Yep. I added this kind of nest in fs_test.c to test both fs and network
rules together.

quoted

is not complete. You can revamp this test and move it to net.c
following the above suggestions, keeping it consistent with other tests
in net.c . You don't need the test_open() nor create_ruleset() helpers.

quoted

This test must failed if we change

"ruleset->access_masks[layer_level] |="

quoted

to "ruleset->access_masks[layer_level] =" in
landlock_add_fs_access_mask() or landlock_add_net_access_mask().

  Do you want to change it? Why?

The kernel code is correct and must not be changed. However, if by
mistake we change it and remove the OR, a test should catch that. We
need a test to assert this assumption.

  OK. I will add additional assert simulating
"ruleset->access_masks[layer_level] =" kernel code.

quoted

quoted

  Fs and network masks are ORed to not intersect with each other.

Yes, they are ORed, and we need a test to check that. Noting is
currently testing this OR (and the different rule type consistency).
I'm suggesting to revamp the layout1.with_net test into
ipv4_tcp.with_fs and make it check ruleset->access_masks[] and rule
addition of different types.

  I will move layout1.with_net test into net.c and rename it. Looks like
  it just needed to add "ruleset->access_masks[layer_level] =" assert
  because the test already has rule addition with different types.

The with_net test doesn't have FS rules, which is the main missing part.
You'll need to rely on the net.c helpers, use the hardcoded paths, and
only handle one access right of each type as I suggested above.

  This is with_net code:

   ....
   /* Adds a network rule. */
	
ASSERT_EQ(0, landlock_add_rule(ruleset_fd_net, LANDLOCK_RULE_NET_PORT,
				       &tcp_bind, 0));

	enforce_ruleset(_metadata, ruleset_fd_net);
	ASSERT_EQ(0, close(ruleset_fd_net));

	ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);

	ASSERT_LE(0, ruleset_fd);
	enforce_ruleset(_metadata, ruleset_fd);
	ASSERT_EQ(0, close(ruleset_fd));
....

It has FS rules - just after ruleset_fd_net rule inforced.
Or maybe I missed something?

quoted

  Do you have any more review updates so far?

That's all for this patch series. :)

   Ok. Thanks.

.