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

From: "Mickaël Salaün" <mic@digikod.net>
To: "Konstantin Meskhidze (A)" <konstantin.meskhidze@huawei.com>
Cc: willemdebruijn.kernel@gmail.com,
	linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
	netfilter-devel@vger.kernel.org, anton.sirazetdinov@huawei.com
Subject: Re: [PATCH v6 00/17] Network support for Landlock
Date: Mon, 29 Aug 2022 15:10:12 +0200	[thread overview]
Message-ID: <f11b7754-b879-20be-0b22-94d94a68de71@digikod.net> (raw)
In-Reply-To: <8eb6509f-8e79-d75c-08f4-80f52c0a26e7@huawei.com>

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