Date: Sat, 1 Jul 2023 17:38:20 +0300
From: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>
To: Mickaël Salaün <mic@...ikod.net>
CC: <willemdebruijn.kernel@...il.com>, <gnoack3000@...il.com>,
	<linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
	<netfilter-devel@...r.kernel.org>, <yusongping@...wei.com>,
	<artem.kuzin@...wei.com>
Subject: Re: [PATCH v11 03/12] landlock: Refactor
 landlock_find_rule/insert_rule



6/26/2023 9:58 PM, Mickaël Salaün пишет:
> Complementary review:
> 
> On 15/05/2023 18:13, Konstantin Meskhidze wrote:
>> Add a new landlock_key union and landlock_id structure to support
>> a socket port rule type. A struct landlock_id identifies a unique entry
>> in a ruleset: either a kernel object (e.g inode) or typed data (e.g TCP
>> port). There is one red-black tree per key type.
>> 
>> This patch also adds is_object_pointer() and get_root() helpers.
>> is_object_pointer() returns true if key type is LANDLOCK_KEY_INODE.
>> get_root() helper returns a red_black tree root pointer according to
>> a key type.
>> 
>> Refactor landlock_insert_rule() and landlock_find_rule() to support coming
>> network modifications. Adding or searching a rule in ruleset can now be
>> done thanks to a Landlock ID argument passed to these helpers.
>> 
>> Co-developed-by: Mickaël Salaün <mic@...ikod.net>
>> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
>> ---
>> 
>> Changes since v10:
>> * None.
>> 
>> Changes since v9:
>> * Splits commit.
>> * Refactors commit message.
>> * Minor fixes.
>> 
>> Changes since v8:
>> * Refactors commit message.
>> * Removes inlining.
>> * Minor fixes.
>> 
>> Changes since v7:
>> * Completes all the new field descriptions landlock_key,
>>    landlock_key_type, landlock_id.
>> * Refactors commit message, adds a co-developer.
>> 
>> Changes since v6:
>> * Adds union landlock_key, enum landlock_key_type, and struct
>>    landlock_id.
>> * Refactors ruleset functions and improves switch/cases: create_rule(),
>>    insert_rule(), get_root(), is_object_pointer(), free_rule(),
>>    landlock_find_rule().
>> * Refactors landlock_append_fs_rule() functions to support new
>>    landlock_id type.
>> 
>> 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      |  21 +++---
>>   security/landlock/ruleset.c | 134 ++++++++++++++++++++++++++----------
>>   security/landlock/ruleset.h |  65 ++++++++++++++---
>>   3 files changed, 166 insertions(+), 54 deletions(-)
>> 
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index a0c9c927fdf9..9a8e70f65a88 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -158,7 +158,9 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>>   			    access_mask_t access_rights)
>>   {
>>   	int err;
>> -	struct landlock_object *object;
>> +	struct landlock_id id = {
>> +		.type = LANDLOCK_KEY_INODE,
>> +	};
>> 
>>   	/* Files only get access rights that make sense. */
>>   	if (!d_is_dir(path->dentry) &&
>> @@ -170,17 +172,17 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>>   	/* Transforms relative access rights to absolute ones. */
>>   	access_rights |= LANDLOCK_MASK_ACCESS_FS &
>>   			 ~landlock_get_fs_access_mask(ruleset, 0);
>> -	object = get_inode_object(d_backing_inode(path->dentry));
>> -	if (IS_ERR(object))
>> -		return PTR_ERR(object);
>> +	id.key.object = get_inode_object(d_backing_inode(path->dentry));
>> +	if (IS_ERR(id.key.object))
>> +		return PTR_ERR(id.key.object);
>>   	mutex_lock(&ruleset->lock);
>> -	err = landlock_insert_rule(ruleset, object, access_rights);
>> +	err = landlock_insert_rule(ruleset, id, access_rights);
>>   	mutex_unlock(&ruleset->lock);
>>   	/*
>>   	 * No need to check for an error because landlock_insert_rule()
>>   	 * increments the refcount for the new object if needed.
>>   	 */
>> -	landlock_put_object(object);
>> +	landlock_put_object(id.key.object);
>>   	return err;
>>   }
>> 
>> @@ -197,6 +199,9 @@ find_rule(const struct landlock_ruleset *const domain,
>>   {
>>   	const struct landlock_rule *rule;
>>   	const struct inode *inode;
>> +	struct landlock_id id = {
>> +		.type = LANDLOCK_KEY_INODE,
>> +	};
>> 
>>   	/* Ignores nonexistent leafs. */
>>   	if (d_is_negative(dentry))
>> @@ -204,8 +209,8 @@ 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));
>> +	id.key.object = rcu_dereference(landlock_inode(inode)->object);
>> +	rule = landlock_find_rule(domain, id);
>>   	rcu_read_unlock();
>>   	return rule;
>>   }
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index 1f3188b4e313..deab37838f5b 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
>> @@ -68,8 +68,18 @@ static void build_check_rule(void)
>>   	BUILD_BUG_ON(rule.num_layers < LANDLOCK_MAX_NUM_LAYERS);
>>   }
>> 
>> +static bool is_object_pointer(const enum landlock_key_type key_type)
>> +{
>> +	switch (key_type) {
>> +	case LANDLOCK_KEY_INODE:
>> +		return true;
>> +	}
>> +	WARN_ON_ONCE(1);
>> +	return false;
>> +}
>> +
>>   static struct landlock_rule *
>> -create_rule(struct landlock_object *const object,
>> +create_rule(const struct landlock_id id,
>>   	    const struct landlock_layer (*const layers)[], const u32 num_layers,
>>   	    const struct landlock_layer *const new_layer)
>>   {
>> @@ -90,8 +100,13 @@ 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 (is_object_pointer(id.type)) {
>> +		/* This should be catched by insert_rule(). */
>> +		WARN_ON_ONCE(!id.key.object);
>> +		landlock_get_object(id.key.object);
>> +	}
>> +
>> +	new_rule->key = id.key;
>>   	new_rule->num_layers = new_num_layers;
>>   	/* Copies the original layer stack. */
>>   	memcpy(new_rule->layers, layers,
>> @@ -102,12 +117,29 @@ create_rule(struct landlock_object *const object,
>>   	return new_rule;
>>   }
>> 
>> -static void free_rule(struct landlock_rule *const rule)
>> +static struct rb_root *get_root(struct landlock_ruleset *const ruleset,
>> +				const enum landlock_key_type key_type)
>> +{
>> +	struct rb_root *root = NULL;
>> +
>> +	switch (key_type) {
>> +	case LANDLOCK_KEY_INODE:
>> +		root = &ruleset->root_inode;
>> +		break;
>> +	}
>> +	if (WARN_ON_ONCE(!root))
>> +		return ERR_PTR(-EINVAL);
>> +	return root;
>> +}
>> +
>> +static void free_rule(struct landlock_rule *const rule,
>> +		      const enum landlock_key_type key_type)
>>   {
>>   	might_sleep();
>>   	if (!rule)
>>   		return;
>> -	landlock_put_object(rule->object);
>> +	if (is_object_pointer(key_type))
>> +		landlock_put_object(rule->key.object);
>>   	kfree(rule);
>>   }
>> 
>> @@ -129,8 +161,8 @@ static void build_check_ruleset(void)
>>    * insert_rule - Create and insert a rule in a ruleset
>>    *
>>    * @ruleset: The ruleset to be updated.
>> - * @object: The object to build the new rule with.  The underlying kernel
>> - *          object must be held by the caller.
>> + * @id: The ID to build the new rule with.  The underlying kernel object, if
>> + *      any, must be held by the caller.
>>    * @layers: One or multiple layers to be copied into the new rule.
>>    * @num_layers: The number of @layers entries.
>>    *
>> @@ -144,26 +176,37 @@ static void build_check_ruleset(void)
>>    * access rights.
>>    */
>>   static int insert_rule(struct landlock_ruleset *const ruleset,
>> -		       struct landlock_object *const object,
>> +		       const struct landlock_id id,
>>   		       const struct landlock_layer (*const layers)[],
>> -		       size_t num_layers)
>> +		       const 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 (is_object_pointer(id.type)) {
>> +		if (WARN_ON_ONCE(!id.key.object))
> 
> This would be simpler:
> 
> if (is_object_pointer(id.type) && WARN_ON_ONCE(!id.key.object))
> 	return -ENOENT;
>  
   Yep. Thank you for the tip.
> 
>> +			return -ENOENT;
>> +	}
>> +
> .

Powered by blists - more mailing lists