[PATCH v11 03/12] landlock: Refactor landlock_find_rule/insert_rule
Konstantin Meskhidze (A)
konstantin.meskhidze at huawei.com
Sat Jul 1 14:38:20 UTC 2023
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 at digikod.net>
>> Signed-off-by: Mickaël Salaün <mic at digikod.net>
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze at huawei.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;
>> + }
>> +
> .
More information about the Linux-security-module-archive
mailing list