[v6,02/17] landlock: refactors landlock_find/insert_rule

Message ID	20220621082313.3330667-3-konstantin.meskhidze@huawei.com (mailing list archive)
State	Not Applicable
Headers	show Return-Path: <netdev-owner@kernel.org> From: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> To: <mic@digikod.net> CC: <willemdebruijn.kernel@gmail.com>, <linux-security-module@vger.kernel.org>, <netdev@vger.kernel.org>, <netfilter-devel@vger.kernel.org>, <yusongping@huawei.com>, <anton.sirazetdinov@huawei.com> Subject: [PATCH v6 02/17] landlock: refactors landlock_find/insert_rule Date: Tue, 21 Jun 2022 16:22:58 +0800 Message-ID: <20220621082313.3330667-3-konstantin.meskhidze@huawei.com> In-Reply-To: <20220621082313.3330667-1-konstantin.meskhidze@huawei.com> References: <20220621082313.3330667-1-konstantin.meskhidze@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII Precedence: bulk
Series	Network support for Landlock \| expand [v6,00/17] Network support for Landlock [v6,01/17] landlock: renames access mask [v6,02/17] landlock: refactors landlock_find/insert_rule [v6,03/17] landlock: refactors merge and inherit functions [v6,04/17] landlock: moves helper functions [v6,05/17] landlock: refactors helper functions [v6,06/17] landlock: refactors landlock_add_rule syscall [v6,07/17] landlock: user space API network support [v6,08/17] landlock: adds support network rules [v6,09/17] landlock: implements TCP network hooks [v6,10/17] seltests/landlock: moves helper function [v6,11/17] seltests/landlock: adds tests for bind() hooks [v6,12/17] seltests/landlock: adds tests for connect() hooks [v6,13/17] seltests/landlock: adds AF_UNSPEC family test [v6,14/17] seltests/landlock: adds rules overlapping test [v6,15/17] seltests/landlock: adds ruleset expanding test [v6,16/17] seltests/landlock: adds invalid input data test [v6,17/17] samples/landlock: adds network demo

Context	Check	Description
netdev/tree_selection	success	Not a local patch, async

Konstantin Meskhidze (A) June 21, 2022, 8:22 a.m. UTC

Adds a new object union to support a socket port
rule type. Refactors landlock_insert_rule() and
landlock_find_rule() to support coming network
modifications. Now adding or searching a rule
in a ruleset depends on a rule_type argument
provided in refactored functions mentioned above.

Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
---

Changes since v5:
* Formats code with clang-format-14.

Changes since v4:
* Refactors insert_rule() and create_rule() functions by deleting
rule_type from their arguments list, it helps to reduce useless code.

Changes since v3:
* Splits commit.
* Refactors landlock_insert_rule and landlock_find_rule functions.
* Rename new_ruleset->root_inode.

---
 security/landlock/fs.c      |   7 ++-
 security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
 security/landlock/ruleset.h |  27 +++++-----
 3 files changed, 96 insertions(+), 43 deletions(-)

--
2.25.1

Mickaël Salaün July 7, 2022, 4:44 p.m. UTC | #1

On 21/06/2022 10:22, Konstantin Meskhidze wrote:
> Adds a new object union to support a socket port
> rule type. Refactors landlock_insert_rule() and
> landlock_find_rule() to support coming network
> modifications. Now adding or searching a rule
> in a ruleset depends on a rule_type argument
> provided in refactored functions mentioned above.
> 
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> ---
> 
> Changes since v5:
> * Formats code with clang-format-14.
> 
> Changes since v4:
> * Refactors insert_rule() and create_rule() functions by deleting
> rule_type from their arguments list, it helps to reduce useless code.
> 
> Changes since v3:
> * Splits commit.
> * Refactors landlock_insert_rule and landlock_find_rule functions.
> * Rename new_ruleset->root_inode.
> 
> ---
>   security/landlock/fs.c      |   7 ++-
>   security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
>   security/landlock/ruleset.h |  27 +++++-----
>   3 files changed, 96 insertions(+), 43 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index e6da08ed99d1..46aedc2a05a8 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>   	if (IS_ERR(object))
>   		return PTR_ERR(object);
>   	mutex_lock(&ruleset->lock);
> -	err = landlock_insert_rule(ruleset, object, access_rights);
> +	err = landlock_insert_rule(ruleset, object, 0, access_rights,
> +				   LANDLOCK_RULE_PATH_BENEATH);
>   	mutex_unlock(&ruleset->lock);
>   	/*
>   	 * No need to check for an error because landlock_insert_rule()
> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const domain,
>   	inode = d_backing_inode(dentry);
>   	rcu_read_lock();
>   	rule = landlock_find_rule(
> -		domain, rcu_dereference(landlock_inode(inode)->object));
> +		domain,
> +		(uintptr_t)rcu_dereference(landlock_inode(inode)->object),
> +		LANDLOCK_RULE_PATH_BENEATH);
>   	rcu_read_unlock();
>   	return rule;
>   }
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index a3fd58d01f09..5f13f8a12aee 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -35,7 +35,7 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>   		return ERR_PTR(-ENOMEM);
>   	refcount_set(&new_ruleset->usage, 1);
>   	mutex_init(&new_ruleset->lock);
> -	new_ruleset->root = RB_ROOT;
> +	new_ruleset->root_inode = RB_ROOT;
>   	new_ruleset->num_layers = num_layers;
>   	/*
>   	 * hierarchy = NULL
> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>   }
> 
>   static struct landlock_rule *
> -create_rule(struct landlock_object *const object,
> +create_rule(struct landlock_object *const object_ptr,
> +	    const uintptr_t object_data,
>   	    const struct landlock_layer (*const layers)[], const u32 num_layers,
>   	    const struct landlock_layer *const new_layer)
>   {
> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>   	if (!new_rule)
>   		return ERR_PTR(-ENOMEM);
>   	RB_CLEAR_NODE(&new_rule->node);
> -	landlock_get_object(object);
> -	new_rule->object = object;
> +
> +	if (object_ptr) {
> +		landlock_get_object(object_ptr);
> +		new_rule->object.ptr = object_ptr;
> +	} else if (object_ptr && object_data) {

Something is wrong with this second check: else + object_ptr?


> +		WARN_ON_ONCE(1);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>   	new_rule->num_layers = new_num_layers;
>   	/* Copies the original layer stack. */
>   	memcpy(new_rule->layers, layers,
> @@ -107,7 +115,7 @@ static void free_rule(struct landlock_rule *const rule)
>   	might_sleep();
>   	if (!rule)
>   		return;
> -	landlock_put_object(rule->object);
> +	landlock_put_object(rule->object.ptr);
>   	kfree(rule);
>   }
> 
> @@ -143,26 +151,42 @@ static void build_check_ruleset(void)
>    * access rights.
>    */
>   static int insert_rule(struct landlock_ruleset *const ruleset,
> -		       struct landlock_object *const object,
> +		       struct landlock_object *const object_ptr,
> +		       uintptr_t object_data, u16 rule_type,

Use landlock_rule_type instead of u16, everywhere. It was part of my 
review/patch two versions ago (with other things), please take them into 
account: 
https://lore.kernel.org/r/92d498f0-c598-7413-6b7c-d19c5aec6cab@digikod.net


>   		       const struct landlock_layer (*const layers)[],
>   		       size_t num_layers)
>   {
>   	struct rb_node **walker_node;
>   	struct rb_node *parent_node = NULL;
>   	struct landlock_rule *new_rule;
> +	struct rb_root *root;
> 
>   	might_sleep();
>   	lockdep_assert_held(&ruleset->lock);
> -	if (WARN_ON_ONCE(!object || !layers))
> +	if (WARN_ON_ONCE(!layers))
>   		return -ENOENT;
> -	walker_node = &(ruleset->root.rb_node);
> +	if (WARN_ON_ONCE(object_ptr && object_data))
> +		return -EINVAL;
> +	/* Chooses rb_tree structure depending on a rule type. */
> +	switch (rule_type) {
> +	case LANDLOCK_RULE_PATH_BENEATH:
> +		if (WARN_ON_ONCE(!object_ptr))
> +			return -ENOENT;
> +		object_data = (uintptr_t)object_ptr;
> +		root = &ruleset->root_inode;
> +		break;
> +	default:
> +		WARN_ON_ONCE(1);
> +		return -EINVAL;
> +	}
> +	walker_node = &root->rb_node;
>   	while (*walker_node) {
>   		struct landlock_rule *const this =
>   			rb_entry(*walker_node, struct landlock_rule, node);
> 
> -		if (this->object != object) {
> +		if (this->object.data != object_data) {
>   			parent_node = *walker_node;
> -			if (this->object < object)
> +			if (this->object.data < object_data)
>   				walker_node = &((*walker_node)->rb_right);
>   			else
>   				walker_node = &((*walker_node)->rb_left);
> @@ -194,11 +218,16 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
>   		 * Intersects access rights when it is a merge between a
>   		 * ruleset and a domain.
>   		 */
> -		new_rule = create_rule(object, &this->layers, this->num_layers,
> -				       &(*layers)[0]);
> +		switch (rule_type) {
> +		case LANDLOCK_RULE_PATH_BENEATH:
> +			new_rule = create_rule(object_ptr, 0, &this->layers,
> +					       this->num_layers, &(*layers)[0]);
> +			break;
> +		}
>   		if (IS_ERR(new_rule))
>   			return PTR_ERR(new_rule);
> -		rb_replace_node(&this->node, &new_rule->node, &ruleset->root);
> +		rb_replace_node(&this->node, &new_rule->node,
> +				&ruleset->root_inode);
>   		free_rule(this);
>   		return 0;
>   	}
> @@ -207,11 +236,15 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
>   	build_check_ruleset();
>   	if (ruleset->num_rules >= LANDLOCK_MAX_NUM_RULES)
>   		return -E2BIG;
> -	new_rule = create_rule(object, layers, num_layers, NULL);
> +	switch (rule_type) {
> +	case LANDLOCK_RULE_PATH_BENEATH:
> +		new_rule = create_rule(object_ptr, 0, layers, num_layers, NULL);
> +		break;
> +	}
>   	if (IS_ERR(new_rule))
>   		return PTR_ERR(new_rule);
>   	rb_link_node(&new_rule->node, parent_node, walker_node);
> -	rb_insert_color(&new_rule->node, &ruleset->root);
> +	rb_insert_color(&new_rule->node, &ruleset->root_inode);
>   	ruleset->num_rules++;
>   	return 0;
>   }
> @@ -229,8 +262,9 @@ static void build_check_layer(void)
> 
>   /* @ruleset must be locked by the caller. */
>   int landlock_insert_rule(struct landlock_ruleset *const ruleset,
> -			 struct landlock_object *const object,
> -			 const access_mask_t access)
> +			 struct landlock_object *const object_ptr,
> +			 const uintptr_t object_data,
> +			 const access_mask_t access, const u16 rule_type)
>   {
>   	struct landlock_layer layers[] = { {
>   		.access = access,
> @@ -239,7 +273,8 @@ int landlock_insert_rule(struct landlock_ruleset *const ruleset,
>   	} };
> 
>   	build_check_layer();
> -	return insert_rule(ruleset, object, &layers, ARRAY_SIZE(layers));
> +	return insert_rule(ruleset, object_ptr, object_data, rule_type, &layers,
> +			   ARRAY_SIZE(layers));
>   }
> 
>   static inline void get_hierarchy(struct landlock_hierarchy *const hierarchy)
> @@ -284,8 +319,8 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>   	dst->access_masks[dst->num_layers - 1] = src->access_masks[0];
> 
>   	/* Merges the @src tree. */
> -	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, &src->root,
> -					     node) {
> +	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
> +					     &src->root_inode, node) {
>   		struct landlock_layer layers[] = { {
>   			.level = dst->num_layers,
>   		} };
> @@ -299,7 +334,8 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>   			goto out_unlock;
>   		}
>   		layers[0].access = walker_rule->layers[0].access;
> -		err = insert_rule(dst, walker_rule->object, &layers,
> +		err = insert_rule(dst, walker_rule->object.ptr, 0,
> +				  LANDLOCK_RULE_PATH_BENEATH, &layers,
>   				  ARRAY_SIZE(layers));
>   		if (err)
>   			goto out_unlock;
> @@ -327,8 +363,9 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
> 
>   	/* Copies the @parent tree. */
>   	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
> -					     &parent->root, node) {
> -		err = insert_rule(child, walker_rule->object,
> +					     &parent->root_inode, node) {
> +		err = insert_rule(child, walker_rule->object.ptr, 0,
> +				  LANDLOCK_RULE_PATH_BENEATH,
>   				  &walker_rule->layers,
>   				  walker_rule->num_layers);
>   		if (err)
> @@ -361,7 +398,8 @@ static void free_ruleset(struct landlock_ruleset *const ruleset)
>   	struct landlock_rule *freeme, *next;
> 
>   	might_sleep();
> -	rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root, node)
> +	rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root_inode,
> +					     node)
>   		free_rule(freeme);
>   	put_hierarchy(ruleset->hierarchy);
>   	kfree(ruleset);
> @@ -453,20 +491,29 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,
>    */
>   const struct landlock_rule *
>   landlock_find_rule(const struct landlock_ruleset *const ruleset,
> -		   const struct landlock_object *const object)
> +		   const uintptr_t object_data, const u16 rule_type)
>   {
>   	const struct rb_node *node;
> 
> -	if (!object)
> +	if (!object_data)
>   		return NULL;
> -	node = ruleset->root.rb_node;
> +
> +	switch (rule_type) {
> +	case LANDLOCK_RULE_PATH_BENEATH:
> +		node = ruleset->root_inode.rb_node;
> +		break;
> +	default:
> +		WARN_ON_ONCE(1);
> +		return NULL;
> +	}
> +
>   	while (node) {
>   		struct landlock_rule *this =
>   			rb_entry(node, struct landlock_rule, node);
> 
> -		if (this->object == object)
> +		if (this->object.data == object_data)
>   			return this;
> -		if (this->object < object)
> +		if (this->object.data < object_data)
>   			node = node->rb_right;
>   		else
>   			node = node->rb_left;
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index bd7ab39859bf..a22d132c32a7 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -53,15 +53,17 @@ struct landlock_rule {
>   	 */
>   	struct rb_node node;
>   	/**
> -	 * @object: Pointer to identify a kernel object (e.g. an inode).  This
> -	 * is used as a key for this ruleset element.  This pointer is set once
> -	 * and never modified.  It always points to an allocated object because
> -	 * each rule increments the refcount of its object.
> -	 */
> -	struct landlock_object *object;
> -	/**
> -	 * @num_layers: Number of entries in @layers.
> +	 * @object: A union to identify either a kernel object (e.g. an inode) or
> +	 * a raw data value (e.g. a network socket port). This is used as a key
> +	 * for this ruleset element. This pointer/@object.ptr/ is set once and
> +	 * never modified. It always points to an allocated object because each
> +	 * rule increments the refcount of its object (for inodes).;
>   	 */
> +	union {
> +		struct landlock_object *ptr;
> +		uintptr_t data;
> +	} object;
> +
>   	u32 num_layers;
>   	/**
>   	 * @layers: Stack of layers, from the latest to the newest, implemented
> @@ -98,7 +100,7 @@ struct landlock_ruleset {
>   	 * nodes.  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;
> +	struct rb_root root_inode;
>   	/**
>   	 * @hierarchy: Enables hierarchy identification even when a parent
>   	 * domain vanishes.  This is needed for the ptrace protection.
> @@ -160,8 +162,9 @@ void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
>   void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
> 
>   int landlock_insert_rule(struct landlock_ruleset *const ruleset,
> -			 struct landlock_object *const object,
> -			 const access_mask_t access);
> +			 struct landlock_object *const object_ptr,
> +			 const uintptr_t object_data,
> +			 const access_mask_t access, const u16 rule_type);
> 
>   struct landlock_ruleset *
>   landlock_merge_ruleset(struct landlock_ruleset *const parent,
> @@ -169,7 +172,7 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,
> 
>   const struct landlock_rule *
>   landlock_find_rule(const struct landlock_ruleset *const ruleset,
> -		   const struct landlock_object *const object);
> +		   const uintptr_t object_data, const u16 rule_type);
> 
>   static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
>   {
> --
> 2.25.1
>

Mickaël Salaün July 7, 2022, 4:46 p.m. UTC | #2

On 21/06/2022 10:22, Konstantin Meskhidze wrote:
> Adds a new object union to support a socket port
> rule type. Refactors landlock_insert_rule() and
> landlock_find_rule() to support coming network
> modifications. Now adding or searching a rule
> in a ruleset depends on a rule_type argument
> provided in refactored functions mentioned above.
> 
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> ---
> 
> Changes since v5:
> * Formats code with clang-format-14.
> 
> Changes since v4:
> * Refactors insert_rule() and create_rule() functions by deleting
> rule_type from their arguments list, it helps to reduce useless code.
> 
> Changes since v3:
> * Splits commit.
> * Refactors landlock_insert_rule and landlock_find_rule functions.
> * Rename new_ruleset->root_inode.
> 
> ---
>   security/landlock/fs.c      |   7 ++-
>   security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
>   security/landlock/ruleset.h |  27 +++++-----
>   3 files changed, 96 insertions(+), 43 deletions(-)

[...]

> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index bd7ab39859bf..a22d132c32a7 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -53,15 +53,17 @@ struct landlock_rule {
>   	 */
>   	struct rb_node node;
>   	/**
> -	 * @object: Pointer to identify a kernel object (e.g. an inode).  This
> -	 * is used as a key for this ruleset element.  This pointer is set once
> -	 * and never modified.  It always points to an allocated object because
> -	 * each rule increments the refcount of its object.
> -	 */
> -	struct landlock_object *object;
> -	/**
> -	 * @num_layers: Number of entries in @layers.
> +	 * @object: A union to identify either a kernel object (e.g. an inode) or
> +	 * a raw data value (e.g. a network socket port). This is used as a key
> +	 * for this ruleset element. This pointer/@object.ptr/ is set once and
> +	 * never modified. It always points to an allocated object because each
> +	 * rule increments the refcount of its object (for inodes).;

Extra ";"

Konstantin Meskhidze (A) July 8, 2022, 12:53 p.m. UTC | #3

7/7/2022 7:44 PM, Mickaël Salaün пишет:
> 
> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>> Adds a new object union to support a socket port
>> rule type. Refactors landlock_insert_rule() and
>> landlock_find_rule() to support coming network
>> modifications. Now adding or searching a rule
>> in a ruleset depends on a rule_type argument
>> provided in refactored functions mentioned above.
>> 
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>> ---
>> 
>> Changes since v5:
>> * Formats code with clang-format-14.
>> 
>> Changes since v4:
>> * Refactors insert_rule() and create_rule() functions by deleting
>> rule_type from their arguments list, it helps to reduce useless code.
>> 
>> Changes since v3:
>> * Splits commit.
>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>> * Rename new_ruleset->root_inode.
>> 
>> ---
>>   security/landlock/fs.c      |   7 ++-
>>   security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
>>   security/landlock/ruleset.h |  27 +++++-----
>>   3 files changed, 96 insertions(+), 43 deletions(-)
>> 
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index e6da08ed99d1..46aedc2a05a8 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>>   	if (IS_ERR(object))
>>   		return PTR_ERR(object);
>>   	mutex_lock(&ruleset->lock);
>> -	err = landlock_insert_rule(ruleset, object, access_rights);
>> +	err = landlock_insert_rule(ruleset, object, 0, access_rights,
>> +				   LANDLOCK_RULE_PATH_BENEATH);
>>   	mutex_unlock(&ruleset->lock);
>>   	/*
>>   	 * No need to check for an error because landlock_insert_rule()
>> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const domain,
>>   	inode = d_backing_inode(dentry);
>>   	rcu_read_lock();
>>   	rule = landlock_find_rule(
>> -		domain, rcu_dereference(landlock_inode(inode)->object));
>> +		domain,
>> +		(uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>> +		LANDLOCK_RULE_PATH_BENEATH);
>>   	rcu_read_unlock();
>>   	return rule;
>>   }
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index a3fd58d01f09..5f13f8a12aee 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -35,7 +35,7 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>>   		return ERR_PTR(-ENOMEM);
>>   	refcount_set(&new_ruleset->usage, 1);
>>   	mutex_init(&new_ruleset->lock);
>> -	new_ruleset->root = RB_ROOT;
>> +	new_ruleset->root_inode = RB_ROOT;
>>   	new_ruleset->num_layers = num_layers;
>>   	/*
>>   	 * hierarchy = NULL
>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>   }
>> 
>>   static struct landlock_rule *
>> -create_rule(struct landlock_object *const object,
>> +create_rule(struct landlock_object *const object_ptr,
>> +	    const uintptr_t object_data,
>>   	    const struct landlock_layer (*const layers)[], const u32 num_layers,
>>   	    const struct landlock_layer *const new_layer)
>>   {
>> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>>   	if (!new_rule)
>>   		return ERR_PTR(-ENOMEM);
>>   	RB_CLEAR_NODE(&new_rule->node);
>> -	landlock_get_object(object);
>> -	new_rule->object = object;
>> +
>> +	if (object_ptr) {
>> +		landlock_get_object(object_ptr);
>> +		new_rule->object.ptr = object_ptr;
>> +	} else if (object_ptr && object_data) {
> 
> Something is wrong with this second check: else + object_ptr?

It was your suggestion to use it like this:
" ....You can also add a WARN_ON_ONCE(object_ptr && object_data)."

Please check it here:
https://lore.kernel.org/linux-security-module/bc44f11f-0eaa-a5f6-c5dc-1d36570f1be1@digikod.net/

> 
> 
>> +		WARN_ON_ONCE(1);
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>>   	new_rule->num_layers = new_num_layers;
>>   	/* Copies the original layer stack. */
>>   	memcpy(new_rule->layers, layers,
>> @@ -107,7 +115,7 @@ static void free_rule(struct landlock_rule *const rule)
>>   	might_sleep();
>>   	if (!rule)
>>   		return;
>> -	landlock_put_object(rule->object);
>> +	landlock_put_object(rule->object.ptr);
>>   	kfree(rule);
>>   }
>> 
>> @@ -143,26 +151,42 @@ static void build_check_ruleset(void)
>>    * access rights.
>>    */
>>   static int insert_rule(struct landlock_ruleset *const ruleset,
>> -		       struct landlock_object *const object,
>> +		       struct landlock_object *const object_ptr,
>> +		       uintptr_t object_data, u16 rule_type,
> 
> Use landlock_rule_type instead of u16, everywhere. It was part of my
> review/patch two versions ago (with other things), please take them into
> account:
> https://lore.kernel.org/r/92d498f0-c598-7413-6b7c-d19c5aec6cab@digikod.net
> 
  Ok. Thanks for the reminder.
> 
>>   		       const struct landlock_layer (*const layers)[],
>>   		       size_t num_layers)
>>   {
>>   	struct rb_node **walker_node;
>>   	struct rb_node *parent_node = NULL;
>>   	struct landlock_rule *new_rule;
>> +	struct rb_root *root;
>> 
>>   	might_sleep();
>>   	lockdep_assert_held(&ruleset->lock);
>> -	if (WARN_ON_ONCE(!object || !layers))
>> +	if (WARN_ON_ONCE(!layers))
>>   		return -ENOENT;
>> -	walker_node = &(ruleset->root.rb_node);
>> +	if (WARN_ON_ONCE(object_ptr && object_data))
>> +		return -EINVAL;
>> +	/* Chooses rb_tree structure depending on a rule type. */
>> +	switch (rule_type) {
>> +	case LANDLOCK_RULE_PATH_BENEATH:
>> +		if (WARN_ON_ONCE(!object_ptr))
>> +			return -ENOENT;
>> +		object_data = (uintptr_t)object_ptr;
>> +		root = &ruleset->root_inode;
>> +		break;
>> +	default:
>> +		WARN_ON_ONCE(1);
>> +		return -EINVAL;
>> +	}
>> +	walker_node = &root->rb_node;
>>   	while (*walker_node) {
>>   		struct landlock_rule *const this =
>>   			rb_entry(*walker_node, struct landlock_rule, node);
>> 
>> -		if (this->object != object) {
>> +		if (this->object.data != object_data) {
>>   			parent_node = *walker_node;
>> -			if (this->object < object)
>> +			if (this->object.data < object_data)
>>   				walker_node = &((*walker_node)->rb_right);
>>   			else
>>   				walker_node = &((*walker_node)->rb_left);
>> @@ -194,11 +218,16 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
>>   		 * Intersects access rights when it is a merge between a
>>   		 * ruleset and a domain.
>>   		 */
>> -		new_rule = create_rule(object, &this->layers, this->num_layers,
>> -				       &(*layers)[0]);
>> +		switch (rule_type) {
>> +		case LANDLOCK_RULE_PATH_BENEATH:
>> +			new_rule = create_rule(object_ptr, 0, &this->layers,
>> +					       this->num_layers, &(*layers)[0]);
>> +			break;
>> +		}
>>   		if (IS_ERR(new_rule))
>>   			return PTR_ERR(new_rule);
>> -		rb_replace_node(&this->node, &new_rule->node, &ruleset->root);
>> +		rb_replace_node(&this->node, &new_rule->node,
>> +				&ruleset->root_inode);
>>   		free_rule(this);
>>   		return 0;
>>   	}
>> @@ -207,11 +236,15 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
>>   	build_check_ruleset();
>>   	if (ruleset->num_rules >= LANDLOCK_MAX_NUM_RULES)
>>   		return -E2BIG;
>> -	new_rule = create_rule(object, layers, num_layers, NULL);
>> +	switch (rule_type) {
>> +	case LANDLOCK_RULE_PATH_BENEATH:
>> +		new_rule = create_rule(object_ptr, 0, layers, num_layers, NULL);
>> +		break;
>> +	}
>>   	if (IS_ERR(new_rule))
>>   		return PTR_ERR(new_rule);
>>   	rb_link_node(&new_rule->node, parent_node, walker_node);
>> -	rb_insert_color(&new_rule->node, &ruleset->root);
>> +	rb_insert_color(&new_rule->node, &ruleset->root_inode);
>>   	ruleset->num_rules++;
>>   	return 0;
>>   }
>> @@ -229,8 +262,9 @@ static void build_check_layer(void)
>> 
>>   /* @ruleset must be locked by the caller. */
>>   int landlock_insert_rule(struct landlock_ruleset *const ruleset,
>> -			 struct landlock_object *const object,
>> -			 const access_mask_t access)
>> +			 struct landlock_object *const object_ptr,
>> +			 const uintptr_t object_data,
>> +			 const access_mask_t access, const u16 rule_type)
>>   {
>>   	struct landlock_layer layers[] = { {
>>   		.access = access,
>> @@ -239,7 +273,8 @@ int landlock_insert_rule(struct landlock_ruleset *const ruleset,
>>   	} };
>> 
>>   	build_check_layer();
>> -	return insert_rule(ruleset, object, &layers, ARRAY_SIZE(layers));
>> +	return insert_rule(ruleset, object_ptr, object_data, rule_type, &layers,
>> +			   ARRAY_SIZE(layers));
>>   }
>> 
>>   static inline void get_hierarchy(struct landlock_hierarchy *const hierarchy)
>> @@ -284,8 +319,8 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>   	dst->access_masks[dst->num_layers - 1] = src->access_masks[0];
>> 
>>   	/* Merges the @src tree. */
>> -	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, &src->root,
>> -					     node) {
>> +	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
>> +					     &src->root_inode, node) {
>>   		struct landlock_layer layers[] = { {
>>   			.level = dst->num_layers,
>>   		} };
>> @@ -299,7 +334,8 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>   			goto out_unlock;
>>   		}
>>   		layers[0].access = walker_rule->layers[0].access;
>> -		err = insert_rule(dst, walker_rule->object, &layers,
>> +		err = insert_rule(dst, walker_rule->object.ptr, 0,
>> +				  LANDLOCK_RULE_PATH_BENEATH, &layers,
>>   				  ARRAY_SIZE(layers));
>>   		if (err)
>>   			goto out_unlock;
>> @@ -327,8 +363,9 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
>> 
>>   	/* Copies the @parent tree. */
>>   	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
>> -					     &parent->root, node) {
>> -		err = insert_rule(child, walker_rule->object,
>> +					     &parent->root_inode, node) {
>> +		err = insert_rule(child, walker_rule->object.ptr, 0,
>> +				  LANDLOCK_RULE_PATH_BENEATH,
>>   				  &walker_rule->layers,
>>   				  walker_rule->num_layers);
>>   		if (err)
>> @@ -361,7 +398,8 @@ static void free_ruleset(struct landlock_ruleset *const ruleset)
>>   	struct landlock_rule *freeme, *next;
>> 
>>   	might_sleep();
>> -	rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root, node)
>> +	rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root_inode,
>> +					     node)
>>   		free_rule(freeme);
>>   	put_hierarchy(ruleset->hierarchy);
>>   	kfree(ruleset);
>> @@ -453,20 +491,29 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,
>>    */
>>   const struct landlock_rule *
>>   landlock_find_rule(const struct landlock_ruleset *const ruleset,
>> -		   const struct landlock_object *const object)
>> +		   const uintptr_t object_data, const u16 rule_type)
>>   {
>>   	const struct rb_node *node;
>> 
>> -	if (!object)
>> +	if (!object_data)
>>   		return NULL;
>> -	node = ruleset->root.rb_node;
>> +
>> +	switch (rule_type) {
>> +	case LANDLOCK_RULE_PATH_BENEATH:
>> +		node = ruleset->root_inode.rb_node;
>> +		break;
>> +	default:
>> +		WARN_ON_ONCE(1);
>> +		return NULL;
>> +	}
>> +
>>   	while (node) {
>>   		struct landlock_rule *this =
>>   			rb_entry(node, struct landlock_rule, node);
>> 
>> -		if (this->object == object)
>> +		if (this->object.data == object_data)
>>   			return this;
>> -		if (this->object < object)
>> +		if (this->object.data < object_data)
>>   			node = node->rb_right;
>>   		else
>>   			node = node->rb_left;
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index bd7ab39859bf..a22d132c32a7 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -53,15 +53,17 @@ struct landlock_rule {
>>   	 */
>>   	struct rb_node node;
>>   	/**
>> -	 * @object: Pointer to identify a kernel object (e.g. an inode).  This
>> -	 * is used as a key for this ruleset element.  This pointer is set once
>> -	 * and never modified.  It always points to an allocated object because
>> -	 * each rule increments the refcount of its object.
>> -	 */
>> -	struct landlock_object *object;
>> -	/**
>> -	 * @num_layers: Number of entries in @layers.
>> +	 * @object: A union to identify either a kernel object (e.g. an inode) or
>> +	 * a raw data value (e.g. a network socket port). This is used as a key
>> +	 * for this ruleset element. This pointer/@object.ptr/ is set once and
>> +	 * never modified. It always points to an allocated object because each
>> +	 * rule increments the refcount of its object (for inodes).;
>>   	 */
>> +	union {
>> +		struct landlock_object *ptr;
>> +		uintptr_t data;
>> +	} object;
>> +
>>   	u32 num_layers;
>>   	/**
>>   	 * @layers: Stack of layers, from the latest to the newest, implemented
>> @@ -98,7 +100,7 @@ struct landlock_ruleset {
>>   	 * nodes.  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;
>> +	struct rb_root root_inode;
>>   	/**
>>   	 * @hierarchy: Enables hierarchy identification even when a parent
>>   	 * domain vanishes.  This is needed for the ptrace protection.
>> @@ -160,8 +162,9 @@ void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
>>   void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
>> 
>>   int landlock_insert_rule(struct landlock_ruleset *const ruleset,
>> -			 struct landlock_object *const object,
>> -			 const access_mask_t access);
>> +			 struct landlock_object *const object_ptr,
>> +			 const uintptr_t object_data,
>> +			 const access_mask_t access, const u16 rule_type);
>> 
>>   struct landlock_ruleset *
>>   landlock_merge_ruleset(struct landlock_ruleset *const parent,
>> @@ -169,7 +172,7 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,
>> 
>>   const struct landlock_rule *
>>   landlock_find_rule(const struct landlock_ruleset *const ruleset,
>> -		   const struct landlock_object *const object);
>> +		   const uintptr_t object_data, const u16 rule_type);
>> 
>>   static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
>>   {
>> --
>> 2.25.1
>> 
> .

Konstantin Meskhidze (A) July 8, 2022, 12:54 p.m. UTC | #4

7/7/2022 7:46 PM, Mickaël Salaün пишет:
> 
> 
> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>> Adds a new object union to support a socket port
>> rule type. Refactors landlock_insert_rule() and
>> landlock_find_rule() to support coming network
>> modifications. Now adding or searching a rule
>> in a ruleset depends on a rule_type argument
>> provided in refactored functions mentioned above.
>> 
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>> ---
>> 
>> Changes since v5:
>> * Formats code with clang-format-14.
>> 
>> Changes since v4:
>> * Refactors insert_rule() and create_rule() functions by deleting
>> rule_type from their arguments list, it helps to reduce useless code.
>> 
>> Changes since v3:
>> * Splits commit.
>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>> * Rename new_ruleset->root_inode.
>> 
>> ---
>>   security/landlock/fs.c      |   7 ++-
>>   security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
>>   security/landlock/ruleset.h |  27 +++++-----
>>   3 files changed, 96 insertions(+), 43 deletions(-)
> 
> [...]
> 
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index bd7ab39859bf..a22d132c32a7 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -53,15 +53,17 @@ struct landlock_rule {
>>   	 */
>>   	struct rb_node node;
>>   	/**
>> -	 * @object: Pointer to identify a kernel object (e.g. an inode).  This
>> -	 * is used as a key for this ruleset element.  This pointer is set once
>> -	 * and never modified.  It always points to an allocated object because
>> -	 * each rule increments the refcount of its object.
>> -	 */
>> -	struct landlock_object *object;
>> -	/**
>> -	 * @num_layers: Number of entries in @layers.
>> +	 * @object: A union to identify either a kernel object (e.g. an inode) or
>> +	 * a raw data value (e.g. a network socket port). This is used as a key
>> +	 * for this ruleset element. This pointer/@object.ptr/ is set once and
>> +	 * never modified. It always points to an allocated object because each
>> +	 * rule increments the refcount of its object (for inodes).;
> 
> Extra ";"

  Yep. It's a silly typo. Thanks.
  Will be removed.
> .

Konstantin Meskhidze (A) July 8, 2022, 1:10 p.m. UTC | #5

7/7/2022 7:44 PM, Mickaël Salaün пишет:
> 
> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>> Adds a new object union to support a socket port
>> rule type. Refactors landlock_insert_rule() and
>> landlock_find_rule() to support coming network
>> modifications. Now adding or searching a rule
>> in a ruleset depends on a rule_type argument
>> provided in refactored functions mentioned above.
>> 
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>> ---
>> 
>> Changes since v5:
>> * Formats code with clang-format-14.
>> 
>> Changes since v4:
>> * Refactors insert_rule() and create_rule() functions by deleting
>> rule_type from their arguments list, it helps to reduce useless code.
>> 
>> Changes since v3:
>> * Splits commit.
>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>> * Rename new_ruleset->root_inode.
>> 
>> ---
>>   security/landlock/fs.c      |   7 ++-
>>   security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
>>   security/landlock/ruleset.h |  27 +++++-----
>>   3 files changed, 96 insertions(+), 43 deletions(-)
>> 
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index e6da08ed99d1..46aedc2a05a8 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>>   	if (IS_ERR(object))
>>   		return PTR_ERR(object);
>>   	mutex_lock(&ruleset->lock);
>> -	err = landlock_insert_rule(ruleset, object, access_rights);
>> +	err = landlock_insert_rule(ruleset, object, 0, access_rights,
>> +				   LANDLOCK_RULE_PATH_BENEATH);
>>   	mutex_unlock(&ruleset->lock);
>>   	/*
>>   	 * No need to check for an error because landlock_insert_rule()
>> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const domain,
>>   	inode = d_backing_inode(dentry);
>>   	rcu_read_lock();
>>   	rule = landlock_find_rule(
>> -		domain, rcu_dereference(landlock_inode(inode)->object));
>> +		domain,
>> +		(uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>> +		LANDLOCK_RULE_PATH_BENEATH);
>>   	rcu_read_unlock();
>>   	return rule;
>>   }
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index a3fd58d01f09..5f13f8a12aee 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -35,7 +35,7 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>>   		return ERR_PTR(-ENOMEM);
>>   	refcount_set(&new_ruleset->usage, 1);
>>   	mutex_init(&new_ruleset->lock);
>> -	new_ruleset->root = RB_ROOT;
>> +	new_ruleset->root_inode = RB_ROOT;
>>   	new_ruleset->num_layers = num_layers;
>>   	/*
>>   	 * hierarchy = NULL
>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>   }
>> 
>>   static struct landlock_rule *
>> -create_rule(struct landlock_object *const object,
>> +create_rule(struct landlock_object *const object_ptr,
>> +	    const uintptr_t object_data,
>>   	    const struct landlock_layer (*const layers)[], const u32 num_layers,
>>   	    const struct landlock_layer *const new_layer)
>>   {
>> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>>   	if (!new_rule)
>>   		return ERR_PTR(-ENOMEM);
>>   	RB_CLEAR_NODE(&new_rule->node);
>> -	landlock_get_object(object);
>> -	new_rule->object = object;
>> +
>> +	if (object_ptr) {
>> +		landlock_get_object(object_ptr);
>> +		new_rule->object.ptr = object_ptr;
>> +	} else if (object_ptr && object_data) {
> 
> Something is wrong with this second check: else + object_ptr?

  Sorry. Do you mean logical error here? I got your point.
  You are right!

  I think it must be refactored like this:

	if (object_ptr && !object_data) {
		landlock_get_object(object_ptr);
		new_rule->object.ptr = object_ptr;
	} else if (object_ptr && object_data) {
		...
	}
Plus, I will add a test for this case.

> 
> 
>> +		WARN_ON_ONCE(1);
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>>   	new_rule->num_layers = new_num_layers;
>>   	/* Copies the original layer stack. */
>>   	memcpy(new_rule->layers, layers,
>> @@ -107,7 +115,7 @@ static void free_rule(struct landlock_rule *const rule)
>>   	might_sleep();
>>   	if (!rule)
>>   		return;
>> -	landlock_put_object(rule->object);
>> +	landlock_put_object(rule->object.ptr);
>>   	kfree(rule);
>>   }
>> 
>> @@ -143,26 +151,42 @@ static void build_check_ruleset(void)
>>    * access rights.
>>    */
>>   static int insert_rule(struct landlock_ruleset *const ruleset,
>> -		       struct landlock_object *const object,
>> +		       struct landlock_object *const object_ptr,
>> +		       uintptr_t object_data, u16 rule_type,
> 
> Use landlock_rule_type instead of u16, everywhere. It was part of my
> review/patch two versions ago (with other things), please take them into
> account:
> https://lore.kernel.org/r/92d498f0-c598-7413-6b7c-d19c5aec6cab@digikod.net
> 
> 
>>   		       const struct landlock_layer (*const layers)[],
>>   		       size_t num_layers)
>>   {
>>   	struct rb_node **walker_node;
>>   	struct rb_node *parent_node = NULL;
>>   	struct landlock_rule *new_rule;
>> +	struct rb_root *root;
>> 
>>   	might_sleep();
>>   	lockdep_assert_held(&ruleset->lock);
>> -	if (WARN_ON_ONCE(!object || !layers))
>> +	if (WARN_ON_ONCE(!layers))
>>   		return -ENOENT;
>> -	walker_node = &(ruleset->root.rb_node);
>> +	if (WARN_ON_ONCE(object_ptr && object_data))
>> +		return -EINVAL;
>> +	/* Chooses rb_tree structure depending on a rule type. */
>> +	switch (rule_type) {
>> +	case LANDLOCK_RULE_PATH_BENEATH:
>> +		if (WARN_ON_ONCE(!object_ptr))
>> +			return -ENOENT;
>> +		object_data = (uintptr_t)object_ptr;
>> +		root = &ruleset->root_inode;
>> +		break;
>> +	default:
>> +		WARN_ON_ONCE(1);
>> +		return -EINVAL;
>> +	}
>> +	walker_node = &root->rb_node;
>>   	while (*walker_node) {
>>   		struct landlock_rule *const this =
>>   			rb_entry(*walker_node, struct landlock_rule, node);
>> 
>> -		if (this->object != object) {
>> +		if (this->object.data != object_data) {
>>   			parent_node = *walker_node;
>> -			if (this->object < object)
>> +			if (this->object.data < object_data)
>>   				walker_node = &((*walker_node)->rb_right);
>>   			else
>>   				walker_node = &((*walker_node)->rb_left);
>> @@ -194,11 +218,16 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
>>   		 * Intersects access rights when it is a merge between a
>>   		 * ruleset and a domain.
>>   		 */
>> -		new_rule = create_rule(object, &this->layers, this->num_layers,
>> -				       &(*layers)[0]);
>> +		switch (rule_type) {
>> +		case LANDLOCK_RULE_PATH_BENEATH:
>> +			new_rule = create_rule(object_ptr, 0, &this->layers,
>> +					       this->num_layers, &(*layers)[0]);
>> +			break;
>> +		}
>>   		if (IS_ERR(new_rule))
>>   			return PTR_ERR(new_rule);
>> -		rb_replace_node(&this->node, &new_rule->node, &ruleset->root);
>> +		rb_replace_node(&this->node, &new_rule->node,
>> +				&ruleset->root_inode);
>>   		free_rule(this);
>>   		return 0;
>>   	}
>> @@ -207,11 +236,15 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
>>   	build_check_ruleset();
>>   	if (ruleset->num_rules >= LANDLOCK_MAX_NUM_RULES)
>>   		return -E2BIG;
>> -	new_rule = create_rule(object, layers, num_layers, NULL);
>> +	switch (rule_type) {
>> +	case LANDLOCK_RULE_PATH_BENEATH:
>> +		new_rule = create_rule(object_ptr, 0, layers, num_layers, NULL);
>> +		break;
>> +	}
>>   	if (IS_ERR(new_rule))
>>   		return PTR_ERR(new_rule);
>>   	rb_link_node(&new_rule->node, parent_node, walker_node);
>> -	rb_insert_color(&new_rule->node, &ruleset->root);
>> +	rb_insert_color(&new_rule->node, &ruleset->root_inode);
>>   	ruleset->num_rules++;
>>   	return 0;
>>   }
>> @@ -229,8 +262,9 @@ static void build_check_layer(void)
>> 
>>   /* @ruleset must be locked by the caller. */
>>   int landlock_insert_rule(struct landlock_ruleset *const ruleset,
>> -			 struct landlock_object *const object,
>> -			 const access_mask_t access)
>> +			 struct landlock_object *const object_ptr,
>> +			 const uintptr_t object_data,
>> +			 const access_mask_t access, const u16 rule_type)
>>   {
>>   	struct landlock_layer layers[] = { {
>>   		.access = access,
>> @@ -239,7 +273,8 @@ int landlock_insert_rule(struct landlock_ruleset *const ruleset,
>>   	} };
>> 
>>   	build_check_layer();
>> -	return insert_rule(ruleset, object, &layers, ARRAY_SIZE(layers));
>> +	return insert_rule(ruleset, object_ptr, object_data, rule_type, &layers,
>> +			   ARRAY_SIZE(layers));
>>   }
>> 
>>   static inline void get_hierarchy(struct landlock_hierarchy *const hierarchy)
>> @@ -284,8 +319,8 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>   	dst->access_masks[dst->num_layers - 1] = src->access_masks[0];
>> 
>>   	/* Merges the @src tree. */
>> -	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, &src->root,
>> -					     node) {
>> +	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
>> +					     &src->root_inode, node) {
>>   		struct landlock_layer layers[] = { {
>>   			.level = dst->num_layers,
>>   		} };
>> @@ -299,7 +334,8 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>   			goto out_unlock;
>>   		}
>>   		layers[0].access = walker_rule->layers[0].access;
>> -		err = insert_rule(dst, walker_rule->object, &layers,
>> +		err = insert_rule(dst, walker_rule->object.ptr, 0,
>> +				  LANDLOCK_RULE_PATH_BENEATH, &layers,
>>   				  ARRAY_SIZE(layers));
>>   		if (err)
>>   			goto out_unlock;
>> @@ -327,8 +363,9 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
>> 
>>   	/* Copies the @parent tree. */
>>   	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
>> -					     &parent->root, node) {
>> -		err = insert_rule(child, walker_rule->object,
>> +					     &parent->root_inode, node) {
>> +		err = insert_rule(child, walker_rule->object.ptr, 0,
>> +				  LANDLOCK_RULE_PATH_BENEATH,
>>   				  &walker_rule->layers,
>>   				  walker_rule->num_layers);
>>   		if (err)
>> @@ -361,7 +398,8 @@ static void free_ruleset(struct landlock_ruleset *const ruleset)
>>   	struct landlock_rule *freeme, *next;
>> 
>>   	might_sleep();
>> -	rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root, node)
>> +	rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root_inode,
>> +					     node)
>>   		free_rule(freeme);
>>   	put_hierarchy(ruleset->hierarchy);
>>   	kfree(ruleset);
>> @@ -453,20 +491,29 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,
>>    */
>>   const struct landlock_rule *
>>   landlock_find_rule(const struct landlock_ruleset *const ruleset,
>> -		   const struct landlock_object *const object)
>> +		   const uintptr_t object_data, const u16 rule_type)
>>   {
>>   	const struct rb_node *node;
>> 
>> -	if (!object)
>> +	if (!object_data)
>>   		return NULL;
>> -	node = ruleset->root.rb_node;
>> +
>> +	switch (rule_type) {
>> +	case LANDLOCK_RULE_PATH_BENEATH:
>> +		node = ruleset->root_inode.rb_node;
>> +		break;
>> +	default:
>> +		WARN_ON_ONCE(1);
>> +		return NULL;
>> +	}
>> +
>>   	while (node) {
>>   		struct landlock_rule *this =
>>   			rb_entry(node, struct landlock_rule, node);
>> 
>> -		if (this->object == object)
>> +		if (this->object.data == object_data)
>>   			return this;
>> -		if (this->object < object)
>> +		if (this->object.data < object_data)
>>   			node = node->rb_right;
>>   		else
>>   			node = node->rb_left;
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index bd7ab39859bf..a22d132c32a7 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -53,15 +53,17 @@ struct landlock_rule {
>>   	 */
>>   	struct rb_node node;
>>   	/**
>> -	 * @object: Pointer to identify a kernel object (e.g. an inode).  This
>> -	 * is used as a key for this ruleset element.  This pointer is set once
>> -	 * and never modified.  It always points to an allocated object because
>> -	 * each rule increments the refcount of its object.
>> -	 */
>> -	struct landlock_object *object;
>> -	/**
>> -	 * @num_layers: Number of entries in @layers.
>> +	 * @object: A union to identify either a kernel object (e.g. an inode) or
>> +	 * a raw data value (e.g. a network socket port). This is used as a key
>> +	 * for this ruleset element. This pointer/@object.ptr/ is set once and
>> +	 * never modified. It always points to an allocated object because each
>> +	 * rule increments the refcount of its object (for inodes).;
>>   	 */
>> +	union {
>> +		struct landlock_object *ptr;
>> +		uintptr_t data;
>> +	} object;
>> +
>>   	u32 num_layers;
>>   	/**
>>   	 * @layers: Stack of layers, from the latest to the newest, implemented
>> @@ -98,7 +100,7 @@ struct landlock_ruleset {
>>   	 * nodes.  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;
>> +	struct rb_root root_inode;
>>   	/**
>>   	 * @hierarchy: Enables hierarchy identification even when a parent
>>   	 * domain vanishes.  This is needed for the ptrace protection.
>> @@ -160,8 +162,9 @@ void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
>>   void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
>> 
>>   int landlock_insert_rule(struct landlock_ruleset *const ruleset,
>> -			 struct landlock_object *const object,
>> -			 const access_mask_t access);
>> +			 struct landlock_object *const object_ptr,
>> +			 const uintptr_t object_data,
>> +			 const access_mask_t access, const u16 rule_type);
>> 
>>   struct landlock_ruleset *
>>   landlock_merge_ruleset(struct landlock_ruleset *const parent,
>> @@ -169,7 +172,7 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,
>> 
>>   const struct landlock_rule *
>>   landlock_find_rule(const struct landlock_ruleset *const ruleset,
>> -		   const struct landlock_object *const object);
>> +		   const uintptr_t object_data, const u16 rule_type);
>> 
>>   static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
>>   {
>> --
>> 2.25.1
>> 
> .

Mickaël Salaün July 8, 2022, 1:56 p.m. UTC | #6

On 08/07/2022 14:53, Konstantin Meskhidze (A) wrote:
> 
> 
> 7/7/2022 7:44 PM, Mickaël Salaün пишет:
>>
>> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>>> Adds a new object union to support a socket port
>>> rule type. Refactors landlock_insert_rule() and
>>> landlock_find_rule() to support coming network
>>> modifications. Now adding or searching a rule
>>> in a ruleset depends on a rule_type argument
>>> provided in refactored functions mentioned above.
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>> ---
>>>
>>> Changes since v5:
>>> * Formats code with clang-format-14.
>>>
>>> Changes since v4:
>>> * Refactors insert_rule() and create_rule() functions by deleting
>>> rule_type from their arguments list, it helps to reduce useless code.
>>>
>>> Changes since v3:
>>> * Splits commit.
>>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>>> * Rename new_ruleset->root_inode.
>>>
>>> ---
>>>   security/landlock/fs.c      |   7 ++-
>>>   security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
>>>   security/landlock/ruleset.h |  27 +++++-----
>>>   3 files changed, 96 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>> index e6da08ed99d1..46aedc2a05a8 100644
>>> --- a/security/landlock/fs.c
>>> +++ b/security/landlock/fs.c
>>> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct 
>>> landlock_ruleset *const ruleset,
>>>       if (IS_ERR(object))
>>>           return PTR_ERR(object);
>>>       mutex_lock(&ruleset->lock);
>>> -    err = landlock_insert_rule(ruleset, object, access_rights);
>>> +    err = landlock_insert_rule(ruleset, object, 0, access_rights,
>>> +                   LANDLOCK_RULE_PATH_BENEATH);
>>>       mutex_unlock(&ruleset->lock);
>>>       /*
>>>        * No need to check for an error because landlock_insert_rule()
>>> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const 
>>> domain,
>>>       inode = d_backing_inode(dentry);
>>>       rcu_read_lock();
>>>       rule = landlock_find_rule(
>>> -        domain, rcu_dereference(landlock_inode(inode)->object));
>>> +        domain,
>>> +        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>>> +        LANDLOCK_RULE_PATH_BENEATH);
>>>       rcu_read_unlock();
>>>       return rule;
>>>   }
>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>> index a3fd58d01f09..5f13f8a12aee 100644
>>> --- a/security/landlock/ruleset.c
>>> +++ b/security/landlock/ruleset.c
>>> @@ -35,7 +35,7 @@ static struct landlock_ruleset 
>>> *create_ruleset(const u32 num_layers)
>>>           return ERR_PTR(-ENOMEM);
>>>       refcount_set(&new_ruleset->usage, 1);
>>>       mutex_init(&new_ruleset->lock);
>>> -    new_ruleset->root = RB_ROOT;
>>> +    new_ruleset->root_inode = RB_ROOT;
>>>       new_ruleset->num_layers = num_layers;
>>>       /*
>>>        * hierarchy = NULL
>>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>>   }
>>>
>>>   static struct landlock_rule *
>>> -create_rule(struct landlock_object *const object,
>>> +create_rule(struct landlock_object *const object_ptr,
>>> +        const uintptr_t object_data,
>>>           const struct landlock_layer (*const layers)[], const u32 
>>> num_layers,
>>>           const struct landlock_layer *const new_layer)
>>>   {
>>> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>>>       if (!new_rule)
>>>           return ERR_PTR(-ENOMEM);
>>>       RB_CLEAR_NODE(&new_rule->node);
>>> -    landlock_get_object(object);
>>> -    new_rule->object = object;
>>> +
>>> +    if (object_ptr) {
>>> +        landlock_get_object(object_ptr);
>>> +        new_rule->object.ptr = object_ptr;
>>> +    } else if (object_ptr && object_data) {
>>
>> Something is wrong with this second check: else + object_ptr?
> 
> It was your suggestion to use it like this:
> " ....You can also add a WARN_ON_ONCE(object_ptr && object_data)."
> 
> Please check it here:
> https://lore.kernel.org/linux-security-module/bc44f11f-0eaa-a5f6-c5dc-1d36570f1be1@digikod.net/ 

Yes, but the error is in the "else", you should write:
if (WARN_ON_ONCE(object_ptr && object_data))
	return ERR_PTR(-EINVAL);

…and this should be before the `if (object_ptr) {` line (to avoid 
erronous landlock_get_object() call), just after the `if (!new_rule)` check.

Mickaël Salaün July 8, 2022, 1:59 p.m. UTC | #7

On 08/07/2022 15:10, Konstantin Meskhidze (A) wrote:
> 
> 
> 7/7/2022 7:44 PM, Mickaël Salaün пишет:
>>
>> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>>> Adds a new object union to support a socket port
>>> rule type. Refactors landlock_insert_rule() and
>>> landlock_find_rule() to support coming network
>>> modifications. Now adding or searching a rule
>>> in a ruleset depends on a rule_type argument
>>> provided in refactored functions mentioned above.
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>> ---
>>>
>>> Changes since v5:
>>> * Formats code with clang-format-14.
>>>
>>> Changes since v4:
>>> * Refactors insert_rule() and create_rule() functions by deleting
>>> rule_type from their arguments list, it helps to reduce useless code.
>>>
>>> Changes since v3:
>>> * Splits commit.
>>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>>> * Rename new_ruleset->root_inode.
>>>
>>> ---
>>>   security/landlock/fs.c      |   7 ++-
>>>   security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
>>>   security/landlock/ruleset.h |  27 +++++-----
>>>   3 files changed, 96 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>> index e6da08ed99d1..46aedc2a05a8 100644
>>> --- a/security/landlock/fs.c
>>> +++ b/security/landlock/fs.c
>>> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct 
>>> landlock_ruleset *const ruleset,
>>>       if (IS_ERR(object))
>>>           return PTR_ERR(object);
>>>       mutex_lock(&ruleset->lock);
>>> -    err = landlock_insert_rule(ruleset, object, access_rights);
>>> +    err = landlock_insert_rule(ruleset, object, 0, access_rights,
>>> +                   LANDLOCK_RULE_PATH_BENEATH);
>>>       mutex_unlock(&ruleset->lock);
>>>       /*
>>>        * No need to check for an error because landlock_insert_rule()
>>> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const 
>>> domain,
>>>       inode = d_backing_inode(dentry);
>>>       rcu_read_lock();
>>>       rule = landlock_find_rule(
>>> -        domain, rcu_dereference(landlock_inode(inode)->object));
>>> +        domain,
>>> +        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>>> +        LANDLOCK_RULE_PATH_BENEATH);
>>>       rcu_read_unlock();
>>>       return rule;
>>>   }
>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>> index a3fd58d01f09..5f13f8a12aee 100644
>>> --- a/security/landlock/ruleset.c
>>> +++ b/security/landlock/ruleset.c
>>> @@ -35,7 +35,7 @@ static struct landlock_ruleset 
>>> *create_ruleset(const u32 num_layers)
>>>           return ERR_PTR(-ENOMEM);
>>>       refcount_set(&new_ruleset->usage, 1);
>>>       mutex_init(&new_ruleset->lock);
>>> -    new_ruleset->root = RB_ROOT;
>>> +    new_ruleset->root_inode = RB_ROOT;
>>>       new_ruleset->num_layers = num_layers;
>>>       /*
>>>        * hierarchy = NULL
>>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>>   }
>>>
>>>   static struct landlock_rule *
>>> -create_rule(struct landlock_object *const object,
>>> +create_rule(struct landlock_object *const object_ptr,
>>> +        const uintptr_t object_data,
>>>           const struct landlock_layer (*const layers)[], const u32 
>>> num_layers,
>>>           const struct landlock_layer *const new_layer)
>>>   {
>>> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>>>       if (!new_rule)
>>>           return ERR_PTR(-ENOMEM);
>>>       RB_CLEAR_NODE(&new_rule->node);
>>> -    landlock_get_object(object);
>>> -    new_rule->object = object;
>>> +
>>> +    if (object_ptr) {
>>> +        landlock_get_object(object_ptr);
>>> +        new_rule->object.ptr = object_ptr;
>>> +    } else if (object_ptr && object_data) {
>>
>> Something is wrong with this second check: else + object_ptr?
> 
>   Sorry. Do you mean logical error here? I got your point.
>   You are right!
> 
>   I think it must be refactored like this:
> 
>      if (object_ptr && !object_data) {
>          landlock_get_object(object_ptr);
>          new_rule->object.ptr = object_ptr;
>      } else if (object_ptr && object_data) {
>          ...
>      }

There is indeed a logical error but this doesn't fix everything. Please 
include my previous suggestion instead.


> Plus, I will add a test for this case.

That would be great but I don't think this code is reachable from user 
space. I think that would require kunit but I may be missing something. 
How would you test this?

Konstantin Meskhidze (A) July 8, 2022, 2:14 p.m. UTC | #8

7/8/2022 4:59 PM, Mickaël Salaün пишет:
> 
> On 08/07/2022 15:10, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 7/7/2022 7:44 PM, Mickaël Salaün пишет:
>>>
>>> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>>>> Adds a new object union to support a socket port
>>>> rule type. Refactors landlock_insert_rule() and
>>>> landlock_find_rule() to support coming network
>>>> modifications. Now adding or searching a rule
>>>> in a ruleset depends on a rule_type argument
>>>> provided in refactored functions mentioned above.
>>>>
>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>> ---
>>>>
>>>> Changes since v5:
>>>> * Formats code with clang-format-14.
>>>>
>>>> Changes since v4:
>>>> * Refactors insert_rule() and create_rule() functions by deleting
>>>> rule_type from their arguments list, it helps to reduce useless code.
>>>>
>>>> Changes since v3:
>>>> * Splits commit.
>>>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>>>> * Rename new_ruleset->root_inode.
>>>>
>>>> ---
>>>>   security/landlock/fs.c      |   7 ++-
>>>>   security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
>>>>   security/landlock/ruleset.h |  27 +++++-----
>>>>   3 files changed, 96 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>>> index e6da08ed99d1..46aedc2a05a8 100644
>>>> --- a/security/landlock/fs.c
>>>> +++ b/security/landlock/fs.c
>>>> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct 
>>>> landlock_ruleset *const ruleset,
>>>>       if (IS_ERR(object))
>>>>           return PTR_ERR(object);
>>>>       mutex_lock(&ruleset->lock);
>>>> -    err = landlock_insert_rule(ruleset, object, access_rights);
>>>> +    err = landlock_insert_rule(ruleset, object, 0, access_rights,
>>>> +                   LANDLOCK_RULE_PATH_BENEATH);
>>>>       mutex_unlock(&ruleset->lock);
>>>>       /*
>>>>        * No need to check for an error because landlock_insert_rule()
>>>> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const 
>>>> domain,
>>>>       inode = d_backing_inode(dentry);
>>>>       rcu_read_lock();
>>>>       rule = landlock_find_rule(
>>>> -        domain, rcu_dereference(landlock_inode(inode)->object));
>>>> +        domain,
>>>> +        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>>>> +        LANDLOCK_RULE_PATH_BENEATH);
>>>>       rcu_read_unlock();
>>>>       return rule;
>>>>   }
>>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>>> index a3fd58d01f09..5f13f8a12aee 100644
>>>> --- a/security/landlock/ruleset.c
>>>> +++ b/security/landlock/ruleset.c
>>>> @@ -35,7 +35,7 @@ static struct landlock_ruleset 
>>>> *create_ruleset(const u32 num_layers)
>>>>           return ERR_PTR(-ENOMEM);
>>>>       refcount_set(&new_ruleset->usage, 1);
>>>>       mutex_init(&new_ruleset->lock);
>>>> -    new_ruleset->root = RB_ROOT;
>>>> +    new_ruleset->root_inode = RB_ROOT;
>>>>       new_ruleset->num_layers = num_layers;
>>>>       /*
>>>>        * hierarchy = NULL
>>>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>>>   }
>>>>
>>>>   static struct landlock_rule *
>>>> -create_rule(struct landlock_object *const object,
>>>> +create_rule(struct landlock_object *const object_ptr,
>>>> +        const uintptr_t object_data,
>>>>           const struct landlock_layer (*const layers)[], const u32 
>>>> num_layers,
>>>>           const struct landlock_layer *const new_layer)
>>>>   {
>>>> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>>>>       if (!new_rule)
>>>>           return ERR_PTR(-ENOMEM);
>>>>       RB_CLEAR_NODE(&new_rule->node);
>>>> -    landlock_get_object(object);
>>>> -    new_rule->object = object;
>>>> +
>>>> +    if (object_ptr) {
>>>> +        landlock_get_object(object_ptr);
>>>> +        new_rule->object.ptr = object_ptr;
>>>> +    } else if (object_ptr && object_data) {
>>>
>>> Something is wrong with this second check: else + object_ptr?
>> 
>>   Sorry. Do you mean logical error here? I got your point.
>>   You are right!
>> 
>>   I think it must be refactored like this:
>> 
>>      if (object_ptr && !object_data) {
>>          landlock_get_object(object_ptr);
>>          new_rule->object.ptr = object_ptr;
>>      } else if (object_ptr && object_data) {
>>          ...
>>      }
> 
> There is indeed a logical error but this doesn't fix everything. Please
> include my previous suggestion instead.
> 
    By the way, in the next commits I have fixed this logic error.
Anyway I will refactor this one also. Thanks.
> 
>> Plus, I will add a test for this case.
> 
> That would be great but I don't think this code is reachable from user
> space. I think that would require kunit but I may be missing something.
> How would you test this?

You are correct. I checked it. It's impossible to reach this line from 
userpace (insert both object_ptr and object_data). But create_rule() 
must be used carefuly by other developers (if any in future). Do you 
think if its possible to have some internal kernel tests that could 
handle this issue?
> .

Konstantin Meskhidze (A) July 8, 2022, 2:14 p.m. UTC | #9

7/8/2022 4:56 PM, Mickaël Salaün пишет:
> 
> On 08/07/2022 14:53, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 7/7/2022 7:44 PM, Mickaël Salaün пишет:
>>>
>>> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>>>> Adds a new object union to support a socket port
>>>> rule type. Refactors landlock_insert_rule() and
>>>> landlock_find_rule() to support coming network
>>>> modifications. Now adding or searching a rule
>>>> in a ruleset depends on a rule_type argument
>>>> provided in refactored functions mentioned above.
>>>>
>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>> ---
>>>>
>>>> Changes since v5:
>>>> * Formats code with clang-format-14.
>>>>
>>>> Changes since v4:
>>>> * Refactors insert_rule() and create_rule() functions by deleting
>>>> rule_type from their arguments list, it helps to reduce useless code.
>>>>
>>>> Changes since v3:
>>>> * Splits commit.
>>>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>>>> * Rename new_ruleset->root_inode.
>>>>
>>>> ---
>>>>   security/landlock/fs.c      |   7 ++-
>>>>   security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
>>>>   security/landlock/ruleset.h |  27 +++++-----
>>>>   3 files changed, 96 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>>> index e6da08ed99d1..46aedc2a05a8 100644
>>>> --- a/security/landlock/fs.c
>>>> +++ b/security/landlock/fs.c
>>>> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct 
>>>> landlock_ruleset *const ruleset,
>>>>       if (IS_ERR(object))
>>>>           return PTR_ERR(object);
>>>>       mutex_lock(&ruleset->lock);
>>>> -    err = landlock_insert_rule(ruleset, object, access_rights);
>>>> +    err = landlock_insert_rule(ruleset, object, 0, access_rights,
>>>> +                   LANDLOCK_RULE_PATH_BENEATH);
>>>>       mutex_unlock(&ruleset->lock);
>>>>       /*
>>>>        * No need to check for an error because landlock_insert_rule()
>>>> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const 
>>>> domain,
>>>>       inode = d_backing_inode(dentry);
>>>>       rcu_read_lock();
>>>>       rule = landlock_find_rule(
>>>> -        domain, rcu_dereference(landlock_inode(inode)->object));
>>>> +        domain,
>>>> +        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>>>> +        LANDLOCK_RULE_PATH_BENEATH);
>>>>       rcu_read_unlock();
>>>>       return rule;
>>>>   }
>>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>>> index a3fd58d01f09..5f13f8a12aee 100644
>>>> --- a/security/landlock/ruleset.c
>>>> +++ b/security/landlock/ruleset.c
>>>> @@ -35,7 +35,7 @@ static struct landlock_ruleset 
>>>> *create_ruleset(const u32 num_layers)
>>>>           return ERR_PTR(-ENOMEM);
>>>>       refcount_set(&new_ruleset->usage, 1);
>>>>       mutex_init(&new_ruleset->lock);
>>>> -    new_ruleset->root = RB_ROOT;
>>>> +    new_ruleset->root_inode = RB_ROOT;
>>>>       new_ruleset->num_layers = num_layers;
>>>>       /*
>>>>        * hierarchy = NULL
>>>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>>>   }
>>>>
>>>>   static struct landlock_rule *
>>>> -create_rule(struct landlock_object *const object,
>>>> +create_rule(struct landlock_object *const object_ptr,
>>>> +        const uintptr_t object_data,
>>>>           const struct landlock_layer (*const layers)[], const u32 
>>>> num_layers,
>>>>           const struct landlock_layer *const new_layer)
>>>>   {
>>>> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>>>>       if (!new_rule)
>>>>           return ERR_PTR(-ENOMEM);
>>>>       RB_CLEAR_NODE(&new_rule->node);
>>>> -    landlock_get_object(object);
>>>> -    new_rule->object = object;
>>>> +
>>>> +    if (object_ptr) {
>>>> +        landlock_get_object(object_ptr);
>>>> +        new_rule->object.ptr = object_ptr;
>>>> +    } else if (object_ptr && object_data) {
>>>
>>> Something is wrong with this second check: else + object_ptr?
>> 
>> It was your suggestion to use it like this:
>> " ....You can also add a WARN_ON_ONCE(object_ptr && object_data)."
>> 
>> Please check it here:
>> https://lore.kernel.org/linux-security-module/bc44f11f-0eaa-a5f6-c5dc-1d36570f1be1@digikod.net/ 
> 
> Yes, but the error is in the "else", you should write:
> if (WARN_ON_ONCE(object_ptr && object_data))
> 	return ERR_PTR(-EINVAL);
> 
> …and this should be before the `if (object_ptr) {` line (to avoid
> erronous landlock_get_object() call), just after the `if (!new_rule)` check.

  By the way, in the next commits I have fixed this logic error.
Anyway I will refactor this one also. Thanks.
> .

Konstantin Meskhidze (A) July 8, 2022, 2:20 p.m. UTC | #10

7/8/2022 4:56 PM, Mickaël Salaün пишет:
> 
> On 08/07/2022 14:53, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 7/7/2022 7:44 PM, Mickaël Salaün пишет:
>>>
>>> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>>>> Adds a new object union to support a socket port
>>>> rule type. Refactors landlock_insert_rule() and
>>>> landlock_find_rule() to support coming network
>>>> modifications. Now adding or searching a rule
>>>> in a ruleset depends on a rule_type argument
>>>> provided in refactored functions mentioned above.
>>>>
>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>> ---
>>>>
>>>> Changes since v5:
>>>> * Formats code with clang-format-14.
>>>>
>>>> Changes since v4:
>>>> * Refactors insert_rule() and create_rule() functions by deleting
>>>> rule_type from their arguments list, it helps to reduce useless code.
>>>>
>>>> Changes since v3:
>>>> * Splits commit.
>>>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>>>> * Rename new_ruleset->root_inode.
>>>>
>>>> ---
>>>>   security/landlock/fs.c      |   7 ++-
>>>>   security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
>>>>   security/landlock/ruleset.h |  27 +++++-----
>>>>   3 files changed, 96 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>>> index e6da08ed99d1..46aedc2a05a8 100644
>>>> --- a/security/landlock/fs.c
>>>> +++ b/security/landlock/fs.c
>>>> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct 
>>>> landlock_ruleset *const ruleset,
>>>>       if (IS_ERR(object))
>>>>           return PTR_ERR(object);
>>>>       mutex_lock(&ruleset->lock);
>>>> -    err = landlock_insert_rule(ruleset, object, access_rights);
>>>> +    err = landlock_insert_rule(ruleset, object, 0, access_rights,
>>>> +                   LANDLOCK_RULE_PATH_BENEATH);
>>>>       mutex_unlock(&ruleset->lock);
>>>>       /*
>>>>        * No need to check for an error because landlock_insert_rule()
>>>> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const 
>>>> domain,
>>>>       inode = d_backing_inode(dentry);
>>>>       rcu_read_lock();
>>>>       rule = landlock_find_rule(
>>>> -        domain, rcu_dereference(landlock_inode(inode)->object));
>>>> +        domain,
>>>> +        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>>>> +        LANDLOCK_RULE_PATH_BENEATH);
>>>>       rcu_read_unlock();
>>>>       return rule;
>>>>   }
>>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>>> index a3fd58d01f09..5f13f8a12aee 100644
>>>> --- a/security/landlock/ruleset.c
>>>> +++ b/security/landlock/ruleset.c
>>>> @@ -35,7 +35,7 @@ static struct landlock_ruleset 
>>>> *create_ruleset(const u32 num_layers)
>>>>           return ERR_PTR(-ENOMEM);
>>>>       refcount_set(&new_ruleset->usage, 1);
>>>>       mutex_init(&new_ruleset->lock);
>>>> -    new_ruleset->root = RB_ROOT;
>>>> +    new_ruleset->root_inode = RB_ROOT;
>>>>       new_ruleset->num_layers = num_layers;
>>>>       /*
>>>>        * hierarchy = NULL
>>>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>>>   }
>>>>
>>>>   static struct landlock_rule *
>>>> -create_rule(struct landlock_object *const object,
>>>> +create_rule(struct landlock_object *const object_ptr,
>>>> +        const uintptr_t object_data,
>>>>           const struct landlock_layer (*const layers)[], const u32 
>>>> num_layers,
>>>>           const struct landlock_layer *const new_layer)
>>>>   {
>>>> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>>>>       if (!new_rule)
>>>>           return ERR_PTR(-ENOMEM);
>>>>       RB_CLEAR_NODE(&new_rule->node);
>>>> -    landlock_get_object(object);
>>>> -    new_rule->object = object;
>>>> +
>>>> +    if (object_ptr) {
>>>> +        landlock_get_object(object_ptr);
>>>> +        new_rule->object.ptr = object_ptr;
>>>> +    } else if (object_ptr && object_data) {
>>>
>>> Something is wrong with this second check: else + object_ptr?
>> 
>> It was your suggestion to use it like this:
>> " ....You can also add a WARN_ON_ONCE(object_ptr && object_data)."
>> 
>> Please check it here:
>> https://lore.kernel.org/linux-security-module/bc44f11f-0eaa-a5f6-c5dc-1d36570f1be1@digikod.net/ 
> 
> Yes, but the error is in the "else", you should write:
> if (WARN_ON_ONCE(object_ptr && object_data))
> 	return ERR_PTR(-EINVAL);
> 
> …and this should be before the `if (object_ptr) {` line (to avoid
> erronous landlock_get_object() call), just after the `if (!new_rule)` check.

   Maybe we could delete this check here cause we have it in the upper 
insert_rule() function??

...
	if (WARN_ON_ONCE(!layers))
		return -ENOENT;
------>	if (WARN_ON_ONCE(object_ptr && object_data))
		return -EINVAL;
	/* Chooses rb_tree structure depending on a rule type. */
	switch (rule_type) {
	case LANDLOCK_RULE_PATH_BENEATH:
		if (WARN_ON_ONCE(!object_ptr))
			return -ENOENT;
		object_data = (uintptr_t)object_ptr;
		root = &ruleset->root_inode;
		break;
	default:
		WARN_ON_ONCE(1);
		return -EINVAL;
	}
...

This is double check here. What do you think?
> .

Mickaël Salaün July 8, 2022, 2:35 p.m. UTC | #11

On 08/07/2022 16:14, Konstantin Meskhidze (A) wrote:
> 
> 
> 7/8/2022 4:59 PM, Mickaël Salaün пишет:
>>
>> On 08/07/2022 15:10, Konstantin Meskhidze (A) wrote:
>>>
>>>
>>> 7/7/2022 7:44 PM, Mickaël Salaün пишет:
>>>>
>>>> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>>>>> Adds a new object union to support a socket port
>>>>> rule type. Refactors landlock_insert_rule() and
>>>>> landlock_find_rule() to support coming network
>>>>> modifications. Now adding or searching a rule
>>>>> in a ruleset depends on a rule_type argument
>>>>> provided in refactored functions mentioned above.
>>>>>
>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>>> ---
>>>>>
>>>>> Changes since v5:
>>>>> * Formats code with clang-format-14.
>>>>>
>>>>> Changes since v4:
>>>>> * Refactors insert_rule() and create_rule() functions by deleting
>>>>> rule_type from their arguments list, it helps to reduce useless code.
>>>>>
>>>>> Changes since v3:
>>>>> * Splits commit.
>>>>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>>>>> * Rename new_ruleset->root_inode.
>>>>>
>>>>> ---
>>>>>   security/landlock/fs.c      |   7 ++-
>>>>>   security/landlock/ruleset.c | 105 
>>>>> ++++++++++++++++++++++++++----------
>>>>>   security/landlock/ruleset.h |  27 +++++-----
>>>>>   3 files changed, 96 insertions(+), 43 deletions(-)
>>>>>
>>>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>>>> index e6da08ed99d1..46aedc2a05a8 100644
>>>>> --- a/security/landlock/fs.c
>>>>> +++ b/security/landlock/fs.c
>>>>> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct 
>>>>> landlock_ruleset *const ruleset,
>>>>>       if (IS_ERR(object))
>>>>>           return PTR_ERR(object);
>>>>>       mutex_lock(&ruleset->lock);
>>>>> -    err = landlock_insert_rule(ruleset, object, access_rights);
>>>>> +    err = landlock_insert_rule(ruleset, object, 0, access_rights,
>>>>> +                   LANDLOCK_RULE_PATH_BENEATH);
>>>>>       mutex_unlock(&ruleset->lock);
>>>>>       /*
>>>>>        * No need to check for an error because landlock_insert_rule()
>>>>> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const 
>>>>> domain,
>>>>>       inode = d_backing_inode(dentry);
>>>>>       rcu_read_lock();
>>>>>       rule = landlock_find_rule(
>>>>> -        domain, rcu_dereference(landlock_inode(inode)->object));
>>>>> +        domain,
>>>>> +        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>>>>> +        LANDLOCK_RULE_PATH_BENEATH);
>>>>>       rcu_read_unlock();
>>>>>       return rule;
>>>>>   }
>>>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>>>> index a3fd58d01f09..5f13f8a12aee 100644
>>>>> --- a/security/landlock/ruleset.c
>>>>> +++ b/security/landlock/ruleset.c
>>>>> @@ -35,7 +35,7 @@ static struct landlock_ruleset 
>>>>> *create_ruleset(const u32 num_layers)
>>>>>           return ERR_PTR(-ENOMEM);
>>>>>       refcount_set(&new_ruleset->usage, 1);
>>>>>       mutex_init(&new_ruleset->lock);
>>>>> -    new_ruleset->root = RB_ROOT;
>>>>> +    new_ruleset->root_inode = RB_ROOT;
>>>>>       new_ruleset->num_layers = num_layers;
>>>>>       /*
>>>>>        * hierarchy = NULL
>>>>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>>>>   }
>>>>>
>>>>>   static struct landlock_rule *
>>>>> -create_rule(struct landlock_object *const object,
>>>>> +create_rule(struct landlock_object *const object_ptr,
>>>>> +        const uintptr_t object_data,
>>>>>           const struct landlock_layer (*const layers)[], const u32 
>>>>> num_layers,
>>>>>           const struct landlock_layer *const new_layer)
>>>>>   {
>>>>> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>>>>>       if (!new_rule)
>>>>>           return ERR_PTR(-ENOMEM);
>>>>>       RB_CLEAR_NODE(&new_rule->node);
>>>>> -    landlock_get_object(object);
>>>>> -    new_rule->object = object;
>>>>> +
>>>>> +    if (object_ptr) {
>>>>> +        landlock_get_object(object_ptr);
>>>>> +        new_rule->object.ptr = object_ptr;
>>>>> +    } else if (object_ptr && object_data) {
>>>>
>>>> Something is wrong with this second check: else + object_ptr?
>>>
>>>   Sorry. Do you mean logical error here? I got your point.
>>>   You are right!
>>>
>>>   I think it must be refactored like this:
>>>
>>>      if (object_ptr && !object_data) {
>>>          landlock_get_object(object_ptr);
>>>          new_rule->object.ptr = object_ptr;
>>>      } else if (object_ptr && object_data) {
>>>          ...
>>>      }
>>
>> There is indeed a logical error but this doesn't fix everything. Please
>> include my previous suggestion instead.
>>
>     By the way, in the next commits I have fixed this logic error.
> Anyway I will refactor this one also. Thanks.
>>
>>> Plus, I will add a test for this case.
>>
>> That would be great but I don't think this code is reachable from user
>> space. I think that would require kunit but I may be missing something.
>> How would you test this?
> 
> You are correct. I checked it. It's impossible to reach this line from 
> userpace (insert both object_ptr and object_data). But create_rule() 
> must be used carefuly by other developers (if any in future). Do you 
> think if its possible to have some internal kernel tests that could 
> handle this issue?

We can use kunit tests for such kernel functions, but in this case I'm 
not sure what could be tested. I started working on bringing kunit tests 
to Landlock but it's not ready yet. Please list all non-userspace tests 
you can think about.

Mickaël Salaün July 8, 2022, 4:57 p.m. UTC | #12

On 08/07/2022 16:20, Konstantin Meskhidze (A) wrote:
> 
> 
> 7/8/2022 4:56 PM, Mickaël Salaün пишет:
>>
>> On 08/07/2022 14:53, Konstantin Meskhidze (A) wrote:
>>>
>>>
>>> 7/7/2022 7:44 PM, Mickaël Salaün пишет:
>>>>
>>>> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>>>>> Adds a new object union to support a socket port
>>>>> rule type. Refactors landlock_insert_rule() and
>>>>> landlock_find_rule() to support coming network
>>>>> modifications. Now adding or searching a rule
>>>>> in a ruleset depends on a rule_type argument
>>>>> provided in refactored functions mentioned above.
>>>>>
>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>>> ---
>>>>>
>>>>> Changes since v5:
>>>>> * Formats code with clang-format-14.
>>>>>
>>>>> Changes since v4:
>>>>> * Refactors insert_rule() and create_rule() functions by deleting
>>>>> rule_type from their arguments list, it helps to reduce useless code.
>>>>>
>>>>> Changes since v3:
>>>>> * Splits commit.
>>>>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>>>>> * Rename new_ruleset->root_inode.
>>>>>
>>>>> ---
>>>>>   security/landlock/fs.c      |   7 ++-
>>>>>   security/landlock/ruleset.c | 105 
>>>>> ++++++++++++++++++++++++++----------
>>>>>   security/landlock/ruleset.h |  27 +++++-----
>>>>>   3 files changed, 96 insertions(+), 43 deletions(-)
>>>>>
>>>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>>>> index e6da08ed99d1..46aedc2a05a8 100644
>>>>> --- a/security/landlock/fs.c
>>>>> +++ b/security/landlock/fs.c
>>>>> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct 
>>>>> landlock_ruleset *const ruleset,
>>>>>       if (IS_ERR(object))
>>>>>           return PTR_ERR(object);
>>>>>       mutex_lock(&ruleset->lock);
>>>>> -    err = landlock_insert_rule(ruleset, object, access_rights);
>>>>> +    err = landlock_insert_rule(ruleset, object, 0, access_rights,
>>>>> +                   LANDLOCK_RULE_PATH_BENEATH);
>>>>>       mutex_unlock(&ruleset->lock);
>>>>>       /*
>>>>>        * No need to check for an error because landlock_insert_rule()
>>>>> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const 
>>>>> domain,
>>>>>       inode = d_backing_inode(dentry);
>>>>>       rcu_read_lock();
>>>>>       rule = landlock_find_rule(
>>>>> -        domain, rcu_dereference(landlock_inode(inode)->object));
>>>>> +        domain,
>>>>> +        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>>>>> +        LANDLOCK_RULE_PATH_BENEATH);
>>>>>       rcu_read_unlock();
>>>>>       return rule;
>>>>>   }
>>>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>>>> index a3fd58d01f09..5f13f8a12aee 100644
>>>>> --- a/security/landlock/ruleset.c
>>>>> +++ b/security/landlock/ruleset.c
>>>>> @@ -35,7 +35,7 @@ static struct landlock_ruleset 
>>>>> *create_ruleset(const u32 num_layers)
>>>>>           return ERR_PTR(-ENOMEM);
>>>>>       refcount_set(&new_ruleset->usage, 1);
>>>>>       mutex_init(&new_ruleset->lock);
>>>>> -    new_ruleset->root = RB_ROOT;
>>>>> +    new_ruleset->root_inode = RB_ROOT;
>>>>>       new_ruleset->num_layers = num_layers;
>>>>>       /*
>>>>>        * hierarchy = NULL
>>>>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>>>>   }
>>>>>
>>>>>   static struct landlock_rule *
>>>>> -create_rule(struct landlock_object *const object,
>>>>> +create_rule(struct landlock_object *const object_ptr,
>>>>> +        const uintptr_t object_data,
>>>>>           const struct landlock_layer (*const layers)[], const u32 
>>>>> num_layers,
>>>>>           const struct landlock_layer *const new_layer)
>>>>>   {
>>>>> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>>>>>       if (!new_rule)
>>>>>           return ERR_PTR(-ENOMEM);
>>>>>       RB_CLEAR_NODE(&new_rule->node);
>>>>> -    landlock_get_object(object);
>>>>> -    new_rule->object = object;
>>>>> +
>>>>> +    if (object_ptr) {
>>>>> +        landlock_get_object(object_ptr);
>>>>> +        new_rule->object.ptr = object_ptr;
>>>>> +    } else if (object_ptr && object_data) {
>>>>
>>>> Something is wrong with this second check: else + object_ptr?
>>>
>>> It was your suggestion to use it like this:
>>> " ....You can also add a WARN_ON_ONCE(object_ptr && object_data)."
>>>
>>> Please check it here:
>>> https://lore.kernel.org/linux-security-module/bc44f11f-0eaa-a5f6-c5dc-1d36570f1be1@digikod.net/ 
>>
>>
>> Yes, but the error is in the "else", you should write:
>> if (WARN_ON_ONCE(object_ptr && object_data))
>>     return ERR_PTR(-EINVAL);
>>
>> …and this should be before the `if (object_ptr) {` line (to avoid
>> erronous landlock_get_object() call), just after the `if (!new_rule)` 
>> check.
> 
>    Maybe we could delete this check here cause we have it in the upper 
> insert_rule() function??
> 
> ...
>      if (WARN_ON_ONCE(!layers))
>          return -ENOENT;
> ------>    if (WARN_ON_ONCE(object_ptr && object_data))
>          return -EINVAL;
>      /* Chooses rb_tree structure depending on a rule type. */
>      switch (rule_type) {
>      case LANDLOCK_RULE_PATH_BENEATH:
>          if (WARN_ON_ONCE(!object_ptr))
>              return -ENOENT;
>          object_data = (uintptr_t)object_ptr;
>          root = &ruleset->root_inode;
>          break;
>      default:
>          WARN_ON_ONCE(1);
>          return -EINVAL;
>      }
> ...
> 
> This is double check here. What do you think?

This check is indeed done twice, and for now create_rule() is only 
called from insert_rule(), but I prefer to keep it in both location to 
not get bitten in the future were it could be called from other 
locations. The compiler may be smart enough to remove the redundant 
checks though.

I'll send a patch to improve this part.

Konstantin Meskhidze (A) July 11, 2022, 8:16 a.m. UTC | #13

7/8/2022 7:57 PM, Mickaël Salaün пишет:
> 
> On 08/07/2022 16:20, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 7/8/2022 4:56 PM, Mickaël Salaün пишет:
>>>
>>> On 08/07/2022 14:53, Konstantin Meskhidze (A) wrote:
>>>>
>>>>
>>>> 7/7/2022 7:44 PM, Mickaël Salaün пишет:
>>>>>
>>>>> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>>>>>> Adds a new object union to support a socket port
>>>>>> rule type. Refactors landlock_insert_rule() and
>>>>>> landlock_find_rule() to support coming network
>>>>>> modifications. Now adding or searching a rule
>>>>>> in a ruleset depends on a rule_type argument
>>>>>> provided in refactored functions mentioned above.
>>>>>>
>>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>>>> ---
>>>>>>
>>>>>> Changes since v5:
>>>>>> * Formats code with clang-format-14.
>>>>>>
>>>>>> Changes since v4:
>>>>>> * Refactors insert_rule() and create_rule() functions by deleting
>>>>>> rule_type from their arguments list, it helps to reduce useless code.
>>>>>>
>>>>>> Changes since v3:
>>>>>> * Splits commit.
>>>>>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>>>>>> * Rename new_ruleset->root_inode.
>>>>>>
>>>>>> ---
>>>>>>   security/landlock/fs.c      |   7 ++-
>>>>>>   security/landlock/ruleset.c | 105 
>>>>>> ++++++++++++++++++++++++++----------
>>>>>>   security/landlock/ruleset.h |  27 +++++-----
>>>>>>   3 files changed, 96 insertions(+), 43 deletions(-)
>>>>>>
>>>>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>>>>> index e6da08ed99d1..46aedc2a05a8 100644
>>>>>> --- a/security/landlock/fs.c
>>>>>> +++ b/security/landlock/fs.c
>>>>>> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct 
>>>>>> landlock_ruleset *const ruleset,
>>>>>>       if (IS_ERR(object))
>>>>>>           return PTR_ERR(object);
>>>>>>       mutex_lock(&ruleset->lock);
>>>>>> -    err = landlock_insert_rule(ruleset, object, access_rights);
>>>>>> +    err = landlock_insert_rule(ruleset, object, 0, access_rights,
>>>>>> +                   LANDLOCK_RULE_PATH_BENEATH);
>>>>>>       mutex_unlock(&ruleset->lock);
>>>>>>       /*
>>>>>>        * No need to check for an error because landlock_insert_rule()
>>>>>> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const 
>>>>>> domain,
>>>>>>       inode = d_backing_inode(dentry);
>>>>>>       rcu_read_lock();
>>>>>>       rule = landlock_find_rule(
>>>>>> -        domain, rcu_dereference(landlock_inode(inode)->object));
>>>>>> +        domain,
>>>>>> +        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>>>>>> +        LANDLOCK_RULE_PATH_BENEATH);
>>>>>>       rcu_read_unlock();
>>>>>>       return rule;
>>>>>>   }
>>>>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>>>>> index a3fd58d01f09..5f13f8a12aee 100644
>>>>>> --- a/security/landlock/ruleset.c
>>>>>> +++ b/security/landlock/ruleset.c
>>>>>> @@ -35,7 +35,7 @@ static struct landlock_ruleset 
>>>>>> *create_ruleset(const u32 num_layers)
>>>>>>           return ERR_PTR(-ENOMEM);
>>>>>>       refcount_set(&new_ruleset->usage, 1);
>>>>>>       mutex_init(&new_ruleset->lock);
>>>>>> -    new_ruleset->root = RB_ROOT;
>>>>>> +    new_ruleset->root_inode = RB_ROOT;
>>>>>>       new_ruleset->num_layers = num_layers;
>>>>>>       /*
>>>>>>        * hierarchy = NULL
>>>>>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>>>>>   }
>>>>>>
>>>>>>   static struct landlock_rule *
>>>>>> -create_rule(struct landlock_object *const object,
>>>>>> +create_rule(struct landlock_object *const object_ptr,
>>>>>> +        const uintptr_t object_data,
>>>>>>           const struct landlock_layer (*const layers)[], const u32 
>>>>>> num_layers,
>>>>>>           const struct landlock_layer *const new_layer)
>>>>>>   {
>>>>>> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>>>>>>       if (!new_rule)
>>>>>>           return ERR_PTR(-ENOMEM);
>>>>>>       RB_CLEAR_NODE(&new_rule->node);
>>>>>> -    landlock_get_object(object);
>>>>>> -    new_rule->object = object;
>>>>>> +
>>>>>> +    if (object_ptr) {
>>>>>> +        landlock_get_object(object_ptr);
>>>>>> +        new_rule->object.ptr = object_ptr;
>>>>>> +    } else if (object_ptr && object_data) {
>>>>>
>>>>> Something is wrong with this second check: else + object_ptr?
>>>>
>>>> It was your suggestion to use it like this:
>>>> " ....You can also add a WARN_ON_ONCE(object_ptr && object_data)."
>>>>
>>>> Please check it here:
>>>> https://lore.kernel.org/linux-security-module/bc44f11f-0eaa-a5f6-c5dc-1d36570f1be1@digikod.net/ 
>>>
>>>
>>> Yes, but the error is in the "else", you should write:
>>> if (WARN_ON_ONCE(object_ptr && object_data))
>>>     return ERR_PTR(-EINVAL);
>>>
>>> …and this should be before the `if (object_ptr) {` line (to avoid
>>> erronous landlock_get_object() call), just after the `if (!new_rule)` 
>>> check.

     Ok. I got it.
>> 
>>    Maybe we could delete this check here cause we have it in the upper 
>> insert_rule() function??
>> 
>> ...
>>      if (WARN_ON_ONCE(!layers))
>>          return -ENOENT;
>> ------>    if (WARN_ON_ONCE(object_ptr && object_data))
>>          return -EINVAL;
>>      /* Chooses rb_tree structure depending on a rule type. */
>>      switch (rule_type) {
>>      case LANDLOCK_RULE_PATH_BENEATH:
>>          if (WARN_ON_ONCE(!object_ptr))
>>              return -ENOENT;
>>          object_data = (uintptr_t)object_ptr;
>>          root = &ruleset->root_inode;
>>          break;
>>      default:
>>          WARN_ON_ONCE(1);
>>          return -EINVAL;
>>      }
>> ...
>> 
>> This is double check here. What do you think?
> 
> This check is indeed done twice, and for now create_rule() is only
> called from insert_rule(), but I prefer to keep it in both location to
> not get bitten in the future were it could be called from other
> locations. The compiler may be smart enough to remove the redundant
> checks though.
> 
> I'll send a patch to improve this part.

  Ok. thanks!!!
> .

Konstantin Meskhidze (A) July 11, 2022, 2:05 p.m. UTC | #14

7/8/2022 5:35 PM, Mickaël Salaün пишет:
> 
> On 08/07/2022 16:14, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 7/8/2022 4:59 PM, Mickaël Salaün пишет:
>>>
>>> On 08/07/2022 15:10, Konstantin Meskhidze (A) wrote:
>>>>
>>>>
>>>> 7/7/2022 7:44 PM, Mickaël Salaün пишет:
>>>>>
>>>>> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>>>>>> Adds a new object union to support a socket port
>>>>>> rule type. Refactors landlock_insert_rule() and
>>>>>> landlock_find_rule() to support coming network
>>>>>> modifications. Now adding or searching a rule
>>>>>> in a ruleset depends on a rule_type argument
>>>>>> provided in refactored functions mentioned above.
>>>>>>
>>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>>>> ---
>>>>>>
>>>>>> Changes since v5:
>>>>>> * Formats code with clang-format-14.
>>>>>>
>>>>>> Changes since v4:
>>>>>> * Refactors insert_rule() and create_rule() functions by deleting
>>>>>> rule_type from their arguments list, it helps to reduce useless code.
>>>>>>
>>>>>> Changes since v3:
>>>>>> * Splits commit.
>>>>>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>>>>>> * Rename new_ruleset->root_inode.
>>>>>>
>>>>>> ---
>>>>>>   security/landlock/fs.c      |   7 ++-
>>>>>>   security/landlock/ruleset.c | 105 
>>>>>> ++++++++++++++++++++++++++----------
>>>>>>   security/landlock/ruleset.h |  27 +++++-----
>>>>>>   3 files changed, 96 insertions(+), 43 deletions(-)
>>>>>>
>>>>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>>>>> index e6da08ed99d1..46aedc2a05a8 100644
>>>>>> --- a/security/landlock/fs.c
>>>>>> +++ b/security/landlock/fs.c
>>>>>> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct 
>>>>>> landlock_ruleset *const ruleset,
>>>>>>       if (IS_ERR(object))
>>>>>>           return PTR_ERR(object);
>>>>>>       mutex_lock(&ruleset->lock);
>>>>>> -    err = landlock_insert_rule(ruleset, object, access_rights);
>>>>>> +    err = landlock_insert_rule(ruleset, object, 0, access_rights,
>>>>>> +                   LANDLOCK_RULE_PATH_BENEATH);
>>>>>>       mutex_unlock(&ruleset->lock);
>>>>>>       /*
>>>>>>        * No need to check for an error because landlock_insert_rule()
>>>>>> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const 
>>>>>> domain,
>>>>>>       inode = d_backing_inode(dentry);
>>>>>>       rcu_read_lock();
>>>>>>       rule = landlock_find_rule(
>>>>>> -        domain, rcu_dereference(landlock_inode(inode)->object));
>>>>>> +        domain,
>>>>>> +        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>>>>>> +        LANDLOCK_RULE_PATH_BENEATH);
>>>>>>       rcu_read_unlock();
>>>>>>       return rule;
>>>>>>   }
>>>>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>>>>> index a3fd58d01f09..5f13f8a12aee 100644
>>>>>> --- a/security/landlock/ruleset.c
>>>>>> +++ b/security/landlock/ruleset.c
>>>>>> @@ -35,7 +35,7 @@ static struct landlock_ruleset 
>>>>>> *create_ruleset(const u32 num_layers)
>>>>>>           return ERR_PTR(-ENOMEM);
>>>>>>       refcount_set(&new_ruleset->usage, 1);
>>>>>>       mutex_init(&new_ruleset->lock);
>>>>>> -    new_ruleset->root = RB_ROOT;
>>>>>> +    new_ruleset->root_inode = RB_ROOT;
>>>>>>       new_ruleset->num_layers = num_layers;
>>>>>>       /*
>>>>>>        * hierarchy = NULL
>>>>>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>>>>>   }
>>>>>>
>>>>>>   static struct landlock_rule *
>>>>>> -create_rule(struct landlock_object *const object,
>>>>>> +create_rule(struct landlock_object *const object_ptr,
>>>>>> +        const uintptr_t object_data,
>>>>>>           const struct landlock_layer (*const layers)[], const u32 
>>>>>> num_layers,
>>>>>>           const struct landlock_layer *const new_layer)
>>>>>>   {
>>>>>> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>>>>>>       if (!new_rule)
>>>>>>           return ERR_PTR(-ENOMEM);
>>>>>>       RB_CLEAR_NODE(&new_rule->node);
>>>>>> -    landlock_get_object(object);
>>>>>> -    new_rule->object = object;
>>>>>> +
>>>>>> +    if (object_ptr) {
>>>>>> +        landlock_get_object(object_ptr);
>>>>>> +        new_rule->object.ptr = object_ptr;
>>>>>> +    } else if (object_ptr && object_data) {
>>>>>
>>>>> Something is wrong with this second check: else + object_ptr?
>>>>
>>>>   Sorry. Do you mean logical error here? I got your point.
>>>>   You are right!
>>>>
>>>>   I think it must be refactored like this:
>>>>
>>>>      if (object_ptr && !object_data) {
>>>>          landlock_get_object(object_ptr);
>>>>          new_rule->object.ptr = object_ptr;
>>>>      } else if (object_ptr && object_data) {
>>>>          ...
>>>>      }
>>>
>>> There is indeed a logical error but this doesn't fix everything. Please
>>> include my previous suggestion instead.
>>>
>>     By the way, in the next commits I have fixed this logic error.
>> Anyway I will refactor this one also. Thanks.
>>>
>>>> Plus, I will add a test for this case.
>>>
>>> That would be great but I don't think this code is reachable from user
>>> space. I think that would require kunit but I may be missing something.
>>> How would you test this?
>> 
>> You are correct. I checked it. It's impossible to reach this line from 
>> userpace (insert both object_ptr and object_data). But create_rule() 
>> must be used carefuly by other developers (if any in future). Do you 
>> think if its possible to have some internal kernel tests that could 
>> handle this issue?
> 
> We can use kunit tests for such kernel functions, but in this case I'm
> not sure what could be tested. I started working on bringing kunit tests
> to Landlock but it's not ready yet. Please list all non-userspace tests
> you can think about.

  I'm thinking about ones that we can't reach from the userspace.
  I analyzed test coverage logs finding lines that are untouched by the 
userspace tests.
  Let's discus this list:

	1. create_rule():  - insert both  object_ptr and object_data.

	2. insert_rule():  - insert both  object_ptr and object_data.
			   - insert NULL (*const layers).
			   - insert layers[0].level != 0.
			   - insert num_layers != 1.
			   - insert default rule_type.
	
	3. tree_merge():   - insert default rule_type.
			   - insert walker_rule->num_layers != 1.
			   - insert walker_rule->layers[0].level != 0.
	
	4. tree_copy():    - insert default rule_type.
	
	5. merge_ruleset:  - insert !dst || !dst->hierarchy.
			   - insert src->num_layers != 1 ||
                                     dst->num_layers < 1.

	6. inherit_ruleset(): - insert child->num_layers <=
				   parent->num_layers.
  			      - insert parent->hierarchy = NULL.

	7. landlock_merge_ruleset(): - insert ruleset = NULL.
				     - insert parent = ruleset

	8. landlock_find_rule(): - insert default rule_type.

  Please your opinion?
> .

Mickaël Salaün July 28, 2022, 2:48 p.m. UTC | #15

On 11/07/2022 16:05, Konstantin Meskhidze (A) wrote:
> 
> 
> 7/8/2022 5:35 PM, Mickaël Salaün пишет:
>>
>> On 08/07/2022 16:14, Konstantin Meskhidze (A) wrote:
>>>
>>>
>>> 7/8/2022 4:59 PM, Mickaël Salaün пишет:
>>>>
>>>> On 08/07/2022 15:10, Konstantin Meskhidze (A) wrote:
>>>>>
>>>>>
>>>>> 7/7/2022 7:44 PM, Mickaël Salaün пишет:
>>>>>>
>>>>>> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>>>>>>> Adds a new object union to support a socket port
>>>>>>> rule type. Refactors landlock_insert_rule() and
>>>>>>> landlock_find_rule() to support coming network
>>>>>>> modifications. Now adding or searching a rule
>>>>>>> in a ruleset depends on a rule_type argument
>>>>>>> provided in refactored functions mentioned above.
>>>>>>>
>>>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes since v5:
>>>>>>> * Formats code with clang-format-14.
>>>>>>>
>>>>>>> Changes since v4:
>>>>>>> * Refactors insert_rule() and create_rule() functions by deleting
>>>>>>> rule_type from their arguments list, it helps to reduce useless code.
>>>>>>>
>>>>>>> Changes since v3:
>>>>>>> * Splits commit.
>>>>>>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>>>>>>> * Rename new_ruleset->root_inode.
>>>>>>>
>>>>>>> ---
>>>>>>>    security/landlock/fs.c      |   7 ++-
>>>>>>>    security/landlock/ruleset.c | 105
>>>>>>> ++++++++++++++++++++++++++----------
>>>>>>>    security/landlock/ruleset.h |  27 +++++-----
>>>>>>>    3 files changed, 96 insertions(+), 43 deletions(-)
>>>>>>>
>>>>>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>>>>>> index e6da08ed99d1..46aedc2a05a8 100644
>>>>>>> --- a/security/landlock/fs.c
>>>>>>> +++ b/security/landlock/fs.c
>>>>>>> @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct
>>>>>>> landlock_ruleset *const ruleset,
>>>>>>>        if (IS_ERR(object))
>>>>>>>            return PTR_ERR(object);
>>>>>>>        mutex_lock(&ruleset->lock);
>>>>>>> -    err = landlock_insert_rule(ruleset, object, access_rights);
>>>>>>> +    err = landlock_insert_rule(ruleset, object, 0, access_rights,
>>>>>>> +                   LANDLOCK_RULE_PATH_BENEATH);
>>>>>>>        mutex_unlock(&ruleset->lock);
>>>>>>>        /*
>>>>>>>         * No need to check for an error because landlock_insert_rule()
>>>>>>> @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const
>>>>>>> domain,
>>>>>>>        inode = d_backing_inode(dentry);
>>>>>>>        rcu_read_lock();
>>>>>>>        rule = landlock_find_rule(
>>>>>>> -        domain, rcu_dereference(landlock_inode(inode)->object));
>>>>>>> +        domain,
>>>>>>> +        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>>>>>>> +        LANDLOCK_RULE_PATH_BENEATH);
>>>>>>>        rcu_read_unlock();
>>>>>>>        return rule;
>>>>>>>    }
>>>>>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>>>>>> index a3fd58d01f09..5f13f8a12aee 100644
>>>>>>> --- a/security/landlock/ruleset.c
>>>>>>> +++ b/security/landlock/ruleset.c
>>>>>>> @@ -35,7 +35,7 @@ static struct landlock_ruleset
>>>>>>> *create_ruleset(const u32 num_layers)
>>>>>>>            return ERR_PTR(-ENOMEM);
>>>>>>>        refcount_set(&new_ruleset->usage, 1);
>>>>>>>        mutex_init(&new_ruleset->lock);
>>>>>>> -    new_ruleset->root = RB_ROOT;
>>>>>>> +    new_ruleset->root_inode = RB_ROOT;
>>>>>>>        new_ruleset->num_layers = num_layers;
>>>>>>>        /*
>>>>>>>         * hierarchy = NULL
>>>>>>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>>>>>>    }
>>>>>>>
>>>>>>>    static struct landlock_rule *
>>>>>>> -create_rule(struct landlock_object *const object,
>>>>>>> +create_rule(struct landlock_object *const object_ptr,
>>>>>>> +        const uintptr_t object_data,
>>>>>>>            const struct landlock_layer (*const layers)[], const u32
>>>>>>> num_layers,
>>>>>>>            const struct landlock_layer *const new_layer)
>>>>>>>    {
>>>>>>> @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
>>>>>>>        if (!new_rule)
>>>>>>>            return ERR_PTR(-ENOMEM);
>>>>>>>        RB_CLEAR_NODE(&new_rule->node);
>>>>>>> -    landlock_get_object(object);
>>>>>>> -    new_rule->object = object;
>>>>>>> +
>>>>>>> +    if (object_ptr) {
>>>>>>> +        landlock_get_object(object_ptr);
>>>>>>> +        new_rule->object.ptr = object_ptr;
>>>>>>> +    } else if (object_ptr && object_data) {
>>>>>>
>>>>>> Something is wrong with this second check: else + object_ptr?
>>>>>
>>>>>    Sorry. Do you mean logical error here? I got your point.
>>>>>    You are right!
>>>>>
>>>>>    I think it must be refactored like this:
>>>>>
>>>>>       if (object_ptr && !object_data) {
>>>>>           landlock_get_object(object_ptr);
>>>>>           new_rule->object.ptr = object_ptr;
>>>>>       } else if (object_ptr && object_data) {
>>>>>           ...
>>>>>       }
>>>>
>>>> There is indeed a logical error but this doesn't fix everything. Please
>>>> include my previous suggestion instead.
>>>>
>>>      By the way, in the next commits I have fixed this logic error.
>>> Anyway I will refactor this one also. Thanks.
>>>>
>>>>> Plus, I will add a test for this case.
>>>>
>>>> That would be great but I don't think this code is reachable from user
>>>> space. I think that would require kunit but I may be missing something.
>>>> How would you test this?
>>>
>>> You are correct. I checked it. It's impossible to reach this line from
>>> userpace (insert both object_ptr and object_data). But create_rule()
>>> must be used carefuly by other developers (if any in future). Do you
>>> think if its possible to have some internal kernel tests that could
>>> handle this issue?
>>
>> We can use kunit tests for such kernel functions, but in this case I'm
>> not sure what could be tested. I started working on bringing kunit tests
>> to Landlock but it's not ready yet. Please list all non-userspace tests
>> you can think about.
> 
>    I'm thinking about ones that we can't reach from the userspace.

Right, some lines cannot be reached by user space because they are logically
not possible but safety checks in case of unexpected kernel code modification
(which, unfortunately, cannot be done at build time).


>    I analyzed test coverage logs finding lines that are untouched by the
> userspace tests.
>    Let's discus this list:
> 
> 	1. create_rule():  - insert both  object_ptr and object_data.

This check is gone with my last patch.

> 
> 	2. insert_rule():  - insert both  object_ptr and object_data.

This check is gone with my last patch.


> 			   - insert NULL (*const layers).
> 			   - insert layers[0].level != 0.
> 			   - insert num_layers != 1.
> 			   - insert default rule_type.
> 	
> 	3. tree_merge():   - insert default rule_type.
> 			   - insert walker_rule->num_layers != 1.
> 			   - insert walker_rule->layers[0].level != 0.
> 	
> 	4. tree_copy():    - insert default rule_type.
> 	
> 	5. merge_ruleset:  - insert !dst || !dst->hierarchy.
> 			   - insert src->num_layers != 1 ||
>                                       dst->num_layers < 1.
> 
> 	6. inherit_ruleset(): - insert child->num_layers <=
> 				   parent->num_layers.
>    			      - insert parent->hierarchy = NULL.
> 
> 	7. landlock_merge_ruleset(): - insert ruleset = NULL.
> 				     - insert parent = ruleset
> 
> 	8. landlock_find_rule(): - insert default rule_type.
> 
>    Please your opinion?

All these checks are enclosed with WARN_ON_ONCE(). I'm not sure it will help
to add kunit tests for them. It could be interesting to add kunit test to
check the behavior of standalone helpers though, e.g. managing rulesets or
trees.

[v6,02/17] landlock: refactors landlock_find/insert_rule

Checks

Commit Message

Comments

Patch