Message-ID: <64e21175-77ab-9a91-2997-43411b93cdaa@digikod.net>
Date: Mon, 26 Jun 2023 20:58:59 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
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

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;


> +			return -ENOENT;
> +	}
> +

Powered by blists - more mailing lists