Re: [PATCH v6 00/17] Network support for Landlock

Mickaël Salaün <mic@xxxxxxxxxxx> · Mon, 29 Aug 2022 15:10:12 +0200

On 27/08/2022 15:30, Konstantin Meskhidze (A) wrote:


7/28/2022 4:17 PM, Mickaël Salaün пишет:

[...]

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 "=" > Is it kunit test? If so, do you want to add this kind of tests in future
landlock versions?

In this sixth patch series, landlock_set_fs_access_mask() was replacing 
the content of access_masks[] whereas landlock_set_net_access_mask() was 
ORing it. It didn't lead to a bug because landlock_set_fs_access_mask() 
was called before landlock_set_net_access_mask(), but it was brittle.

Anyway, it was a good reminder to add a test to check that filesystem 
and network restrictions work well together. This can be added as a 
basic filesystem test using a ruleset handling network restrictions but 
no network rule (in fs_test.c), and as a basic network test using a 
ruleset handling filesystem restrictions but no filestem rule (in 
net_test.c).

This could also be part of a kunit test in the future.


+	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 "="
The same above.
I'm going add invalid network attribute checking into TEST_F(socket,
inval) test in coming patch.

Good






