Date:   Thu, 28 Jul 2022 15:17:43 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
Cc:     willemdebruijn.kernel@...il.com,
        linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org, yusongping@...wei.com,
        anton.sirazetdinov@...wei.com
Subject: Re: [PATCH v6 00/17] Network support for Landlock


On 27/07/2022 21:54, Mickaël Salaün wrote:
> 
> 
> On 26/07/2022 19:43, Mickaël Salaün wrote:
>>
>> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>>> Hi,
>>> This is a new V6 patch related to Landlock LSM network confinement.
>>> It is based on the latest landlock-wip branch on top of v5.19-rc2:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=landlock-wip
>>>
>>> It brings refactoring of previous patch version V5:
>>>       - Fixes some logic errors and typos.
>>>       - Adds additional FIXTURE_VARIANT and FIXTURE_VARIANT_ADD helpers
>>>       to support both ip4 and ip6 families and shorten seltests' code.
>>>       - Makes TCP sockets confinement support optional in sandboxer demo.
>>>       - Formats the code with clang-format-14
>>>
>>> All test were run in QEMU evironment and compiled with
>>>    -static flag.
>>>    1. network_test: 18/18 tests passed.
>>>    2. base_test: 7/7 tests passed.
>>>    3. fs_test: 59/59 tests passed.
>>>    4. ptrace_test: 8/8 tests passed.
>>>
>>> Still have issue with base_test were compiled without -static flag
>>> (landlock-wip branch without network support)
>>> 1. base_test: 6/7 tests passed.
>>>    Error:
>>>    #  RUN           global.inconsistent_attr ...
>>>    # base_test.c:54:inconsistent_attr:Expected ENOMSG (42) == errno (22)
>>>    # inconsistent_attr: Test terminated by assertion
>>>    #          FAIL  global.inconsistent_attr
>>> not ok 1 global.inconsistent_attr
>>>
>>> LCOV - code coverage report:
>>>               Hit  Total  Coverage
>>> Lines:      952  1010    94.3 %
>>> Functions:  79   82      96.3 %
>>>
>>> Previous versions:
>>> v5:
>>> https://lore.kernel.org/linux-security-module/20220516152038.39594-1-konstantin.meskhidze@huawei.com
>>> v4:
>>> https://lore.kernel.org/linux-security-module/20220309134459.6448-1-konstantin.meskhidze@huawei.com/
>>> v3:
>>> https://lore.kernel.org/linux-security-module/20220124080215.265538-1-konstantin.meskhidze@huawei.com/
>>> v2:
>>> https://lore.kernel.org/linux-security-module/20211228115212.703084-1-konstantin.meskhidze@huawei.com/
>>> v1:
>>> https://lore.kernel.org/linux-security-module/20211210072123.386713-1-konstantin.meskhidze@huawei.com/
>>>
>>> Konstantin Meskhidze (17):
>>>     landlock: renames access mask
>>>     landlock: refactors landlock_find/insert_rule
>>>     landlock: refactors merge and inherit functions
>>>     landlock: moves helper functions
>>>     landlock: refactors helper functions
>>>     landlock: refactors landlock_add_rule syscall
>>>     landlock: user space API network support
>>>     landlock: adds support network rules
>>>     landlock: implements TCP network hooks
>>>     seltests/landlock: moves helper function
>>>     seltests/landlock: adds tests for bind() hooks
>>>     seltests/landlock: adds tests for connect() hooks
>>>     seltests/landlock: adds AF_UNSPEC family test
>>>     seltests/landlock: adds rules overlapping test
>>>     seltests/landlock: adds ruleset expanding test
>>>     seltests/landlock: adds invalid input data test
>>>     samples/landlock: adds network demo
>>>
>>>    include/uapi/linux/landlock.h               |  49 ++
>>>    samples/landlock/sandboxer.c                | 118 ++-
>>>    security/landlock/Kconfig                   |   1 +
>>>    security/landlock/Makefile                  |   2 +
>>>    security/landlock/fs.c                      | 162 +---
>>>    security/landlock/limits.h                  |   8 +-
>>>    security/landlock/net.c                     | 155 ++++
>>>    security/landlock/net.h                     |  26 +
>>>    security/landlock/ruleset.c                 | 448 +++++++++--
>>>    security/landlock/ruleset.h                 |  91 ++-
>>>    security/landlock/setup.c                   |   2 +
>>>    security/landlock/syscalls.c                | 168 +++--
>>>    tools/testing/selftests/landlock/common.h   |  10 +
>>>    tools/testing/selftests/landlock/config     |   4 +
>>>    tools/testing/selftests/landlock/fs_test.c  |  10 -
>>>    tools/testing/selftests/landlock/net_test.c | 774 ++++++++++++++++++++
>>>    16 files changed, 1737 insertions(+), 291 deletions(-)
>>>    create mode 100644 security/landlock/net.c
>>>    create mode 100644 security/landlock/net.h
>>>    create mode 100644 tools/testing/selftests/landlock/net_test.c
>>>
>>> -- 
>>> 2.25.1
>>>
>>
>> I did a thorough review of all the code. I found that the main issue
>> with this version is that we stick to the layers limit whereas it is
>> only relevant for filesystem hierarchies. You'll find in the following
>> patch miscellaneous fixes and improvement, with some TODOs to get rid of
>> this layer limit. We'll need a test to check that too. You'll need to
>> integrate this diff into your patches though.
> 
> You can find the related patch here:
> https://git.kernel.org/mic/c/8f4104b3dc59e7f110c9b83cdf034d010a2d006f

Thinking more about the layer limit, I think you should keep your
changes and continue using ruleset.access_masks . Indeed, while it might
be a good thing to not be limited by the 16 layers, it would be an issue
with the upcoming audit feature, i.e. being able to point to the ruleset
responsible for a denied access. Here is a new patch (on top of the
other) to improve the current code:
https://git.kernel.org/mic/c/7d6cf40a6f81adf607ad3cc17aaa11e256beeea4


* Add a new access_masks_t for struct ruleset.  This is now u16 but
   would soon need to change to u32 (because of the upcoming
   LANDLOCK_ACCESS_FS_TRUNCATE).  Set back rules' access_masks_t to u16,
   it should be enough for a while.
* Rename LANDLOCK_MASK_SHIFT_NET to LANDLOCK_SHIFT_ACCESS_NET and use
   LANDLOCK_NUM_ACCESS_FS as value.
* Rename landlock_set_*_access_mask() to landlock_add_*_access_mask()
   because it OR values.
* Make landlock_add_*_access_mask() more resilient incorrect values.
* Rename some variable to make them more consistent with the rest of the
   code.
* Add and update kernel documentation.
* Remove unused code.
---
  security/landlock/limits.h  |   8 +-
  security/landlock/ruleset.c |  31 +++-----
  security/landlock/ruleset.h | 150 +++++++++++++++++++++---------------
  3 files changed, 105 insertions(+), 84 deletions(-)

diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 660335258466..e50a12c1b797 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -21,15 +21,13 @@
  #define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_REFER
  #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
  #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
+#define LANDLOCK_SHIFT_ACCESS_FS	0
  
  #define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_CONNECT_TCP
  #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
-// TODO: LANDLOCK_MASK_SHIFT_NET will not be useful with the new
-// ruleset->net_access_mask
-#define LANDLOCK_MASK_SHIFT_NET		16
-
-#define LANDLOCK_RULE_TYPE_NUM		LANDLOCK_RULE_NET_SERVICE
+#define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
  
  /* clang-format on */
+
  #endif /* _SECURITY_LANDLOCK_LIMITS_H */
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index e7555b16069a..1b3433ea4c61 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -47,21 +47,21 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
  }
  
  struct landlock_ruleset *
-landlock_create_ruleset(const access_mask_t access_mask_fs,
-			const access_mask_t access_mask_net)
+landlock_create_ruleset(const access_mask_t fs_access_mask,
+			const access_mask_t net_access_mask)
  {
  	struct landlock_ruleset *new_ruleset;
  
  	/* Informs about useless ruleset. */
-	if (!access_mask_fs && !access_mask_net)
+	if (!fs_access_mask && !net_access_mask)
  		return ERR_PTR(-ENOMSG);
  	new_ruleset = create_ruleset(1);
  	if (IS_ERR(new_ruleset))
  		return new_ruleset;
-	if (access_mask_fs)
-		landlock_set_fs_access_mask(new_ruleset, access_mask_fs, 0);
-	if (access_mask_net)
-		landlock_set_net_access_mask(new_ruleset, access_mask_net, 0);
+	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);
  	return new_ruleset;
  }
  
@@ -160,13 +160,14 @@ static void build_check_ruleset(void)
  		.num_rules = ~0,
  		.num_layers = ~0,
  	};
-	typeof(ruleset.access_masks[0]) fs_access_mask = ~0;
-	typeof(ruleset.access_masks[0]) net_access_mask = ~0;
+	typeof(ruleset.access_masks[0]) access_masks = ~0;
  
  	BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES);
  	BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
-	BUILD_BUG_ON(fs_access_mask < LANDLOCK_MASK_ACCESS_FS);
-	BUILD_BUG_ON(net_access_mask < LANDLOCK_MASK_ACCESS_NET);
+	BUILD_BUG_ON(access_masks <
+		     (LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) +
+			     (LANDLOCK_MASK_ACCESS_NET
+			      << LANDLOCK_SHIFT_ACCESS_NET));
  }
  
  /**
@@ -250,8 +251,6 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
  		 * Intersects access rights when it is a merge between a
  		 * ruleset and a domain.
  		 */
-		// TODO: Don't create a new rule but AND accesses (of the first
-		// and only layer) if !is_object_pointer(id.type)
  		new_rule = create_rule(id, &this->layers, this->num_layers,
  				       &(*layers)[0]);
  		if (IS_ERR(new_rule))
@@ -332,7 +331,6 @@ static int merge_tree(struct landlock_ruleset *const dst,
  	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, src_root,
  					     node) {
  		struct landlock_layer layers[] = { {
-			// TODO: Set level to 1 if !is_object_pointer(key_type).
  			.level = dst->num_layers,
  		} };
  		const struct landlock_id id = {
@@ -531,7 +529,6 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,
  		if (parent->num_layers >= LANDLOCK_MAX_NUM_LAYERS)
  			return ERR_PTR(-E2BIG);
  		num_layers = parent->num_layers + 1;
-		// TODO: Don't increment num_layers if RB_EMPTY_ROOT(&ruleset->root_inode).
  	} else {
  		num_layers = 1;
  	}
@@ -698,10 +695,6 @@ access_mask_t init_layer_masks(const struct landlock_ruleset *const domain,
  
  	switch (key_type) {
  	case LANDLOCK_KEY_INODE:
-		// XXX: landlock_get_fs_access_mask() should not be removed
-		// once we use ruleset->net_access_mask, and we can then
-		// replace the @key_type argument with num_access to make the
-		// code simpler.
  		get_access_mask = landlock_get_fs_access_mask;
  		num_access = LANDLOCK_NUM_ACCESS_FS;
  		break;
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index 59229be378d6..669de66094ed 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -19,8 +19,8 @@
  #include "limits.h"
  #include "object.h"
  
-// TODO: get back to u16 thanks to ruleset->net_access_mask
-typedef u32 access_mask_t;
+/* Rule access mask. */
+typedef u16 access_mask_t;
  /* Makes sure all filesystem access rights can be stored. */
  static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
  /* Makes sure all network access rights can be stored. */
@@ -28,6 +28,12 @@ static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
  /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
  static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
  
+/* Ruleset access masks. */
+typedef u16 access_masks_t;
+/* Makes sure all ruleset access rights can be stored. */
+static_assert(BITS_PER_TYPE(access_masks_t) >=
+	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
+
  typedef u16 layer_mask_t;
  /* Makes sure all layers can be checked. */
  static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS);
@@ -47,16 +53,33 @@ struct landlock_layer {
  	access_mask_t access;
  };
  
+/**
+ * union landlock_key - Key of a ruleset's red-black tree
+ */
  union landlock_key {
  	struct landlock_object *object;
  	uintptr_t data;
  };
  
+/**
+ * enum landlock_key_type - Type of &union landlock_key
+ */
  enum landlock_key_type {
+	/**
+	 * @LANDLOCK_KEY_INODE: Type of &landlock_ruleset.root_inode's node
+	 * keys.
+	 */
  	LANDLOCK_KEY_INODE = 1,
+	/**
+	 * @LANDLOCK_KEY_NET_PORT: Type of &landlock_ruleset.root_net_port's
+	 * node keys.
+	 */
  	LANDLOCK_KEY_NET_PORT = 2,
  };
  
+/**
+ * struct landlock_id - Unique rule identifier for a ruleset
+ */
  struct landlock_id {
  	union landlock_key key;
  	const enum landlock_key_type type;
@@ -113,15 +136,17 @@ struct landlock_hierarchy {
   */
  struct landlock_ruleset {
  	/**
-	 * @root: Root of a red-black tree containing &struct landlock_rule
-	 * nodes.  Once a ruleset is tied to a process (i.e. as a domain), this
-	 * tree is immutable until @usage reaches zero.
+	 * @root_inode: Root of a red-black tree containing &struct
+	 * landlock_rule nodes with inode object.  Once a ruleset is tied to a
+	 * process (i.e. as a domain), this tree is immutable until @usage
+	 * reaches zero.
  	 */
  	struct rb_root root_inode;
  	/**
-	 * @root_net_port: Root of a red-black tree containing object nodes
-	 * for network port. Once a ruleset is tied to a process (i.e. as a domain),
-	 * this tree is immutable until @usage reaches zero.
+	 * @root_net_port: Root of a red-black tree containing &struct
+	 * landlock_rule nodes with network port. Once a ruleset is tied to a
+	 * process (i.e. as a domain), this tree is immutable until @usage
+	 * reaches zero.
  	 */
  	struct rb_root root_net_port;
  	/**
@@ -162,32 +187,25 @@ struct landlock_ruleset {
  			 */
  			u32 num_layers;
  			/**
-			 * TODO: net_access_mask: Contains the subset of network
-			 * actions that are restricted by a ruleset.
-			 */
-			access_mask_t net_access_mask;
-			/**
-			 * @access_masks: Contains the subset of filesystem
-			 * actions that are restricted by a ruleset.  A domain
-			 * saves all layers of merged rulesets in a stack
-			 * (FAM), starting from the first layer to the last
-			 * one.  These layers are used when merging rulesets,
-			 * for user space backward compatibility (i.e.
-			 * future-proof), and to properly handle merged
+			 * @access_masks: Contains the subset of filesystem and
+			 * network actions that are restricted by a ruleset.
+			 * A domain saves all layers of merged rulesets in a
+			 * stack (FAM), starting from the first layer to the
+			 * last one.  These layers are used when merging
+			 * rulesets, for user space backward compatibility
+			 * (i.e. future-proof), and to properly handle merged
  			 * rulesets without overlapping access rights.  These
  			 * layers are set once and never changed for the
  			 * lifetime of the ruleset.
  			 */
-			// TODO: rename (back) to fs_access_mask because layers
-			// are only useful for file hierarchies.
-			access_mask_t access_masks[];
+			access_masks_t access_masks[];
  		};
  	};
  };
  
  struct landlock_ruleset *
-landlock_create_ruleset(const access_mask_t access_mask_fs,
-			const access_mask_t access_mask_net);
+landlock_create_ruleset(const access_mask_t fs_access_mask,
+			const access_mask_t net_access_mask);
  
  void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
  void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
@@ -210,41 +228,7 @@ static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
  		refcount_inc(&ruleset->usage);
  }
  
-// TODO: These helpers should not be required thanks to the new ruleset->net_access_mask.
-/* A helper function to set a filesystem mask. */
-static inline void
-landlock_set_fs_access_mask(struct landlock_ruleset *ruleset,
-			    const access_mask_t access_mask_fs, u16 mask_level)
-{
-	ruleset->access_masks[mask_level] = access_mask_fs;
-}
-
-/* A helper function to get a filesystem mask. */
-static inline u32
-landlock_get_fs_access_mask(const struct landlock_ruleset *ruleset,
-			    u16 mask_level)
-{
-	return (ruleset->access_masks[mask_level] & LANDLOCK_MASK_ACCESS_FS);
-}
-
-/* A helper function to set a network mask. */
-static inline void
-landlock_set_net_access_mask(struct landlock_ruleset *ruleset,
-			     const access_mask_t access_mask_net,
-			     u16 mask_level)
-{
-	ruleset->access_masks[mask_level] |=
-		(access_mask_net << LANDLOCK_MASK_SHIFT_NET);
-}
-
-/* A helper function to get a network mask. */
-static inline u32
-landlock_get_net_access_mask(const struct landlock_ruleset *ruleset,
-			     u16 mask_level)
-{
-	return (ruleset->access_masks[mask_level] >> LANDLOCK_MASK_SHIFT_NET);
-}
-
+// TODO: Remove if only relevant for fs.c
  access_mask_t get_handled_accesses(const struct landlock_ruleset *const domain,
  				   const u16 rule_type, const u16 num_access);
  
@@ -258,4 +242,50 @@ access_mask_t init_layer_masks(const struct landlock_ruleset *const domain,
  			       layer_mask_t (*const layer_masks)[],
  			       const enum landlock_key_type key_type);
  
+static inline void
+landlock_add_fs_access_mask(struct landlock_ruleset *const ruleset,
+			    const access_mask_t fs_access_mask,
+			    const u16 layer_level)
+{
+	access_mask_t fs_mask = fs_access_mask & LANDLOCK_MASK_ACCESS_FS;
+
+	/* Should already be checked in sys_landlock_create_ruleset(). */
+	WARN_ON_ONCE(fs_access_mask != fs_mask);
+	// TODO: Add tests to check "|=" and not "="
+	ruleset->access_masks[layer_level] |=
+		(fs_mask << LANDLOCK_SHIFT_ACCESS_FS);
+}
+
+static inline void
+landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
+			     const access_mask_t net_access_mask,
+			     const u16 layer_level)
+{
+	access_mask_t net_mask = net_access_mask & LANDLOCK_MASK_ACCESS_NET;
+
+	/* Should already be checked in sys_landlock_create_ruleset(). */
+	WARN_ON_ONCE(net_access_mask != net_mask);
+	// TODO: Add tests to check "|=" and not "="
+	ruleset->access_masks[layer_level] |=
+		(net_mask << LANDLOCK_SHIFT_ACCESS_NET);
+}
+
+static inline access_mask_t
+landlock_get_fs_access_mask(const struct landlock_ruleset *const ruleset,
+			    const u16 layer_level)
+{
+	return (ruleset->access_masks[layer_level] >>
+		LANDLOCK_SHIFT_ACCESS_FS) &
+	       LANDLOCK_MASK_ACCESS_FS;
+}
+
+static inline access_mask_t
+landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
+			     const u16 layer_level)
+{
+	return (ruleset->access_masks[layer_level] >>
+		LANDLOCK_SHIFT_ACCESS_NET) &
+	       LANDLOCK_MASK_ACCESS_NET;
+}
+
  #endif /* _SECURITY_LANDLOCK_RULESET_H */

Powered by blists - more mailing lists