| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c2276d91-3296-8ad2-ba2a-0ba37ec40423@huawei.com>
Date: Mon, 28 Nov 2022 05:58:38 +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>, <artem.kuzin@...wei.com>
Subject: Re: [PATCH v8 02/12] landlock: Refactor
landlock_find_rule/insert_rule
11/17/2022 9:41 PM, Mickaël Salaün пишет:
>
> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>> Adds a new landlock_key union and landlock_id structure to support
>
> Add a new…
Ok. Thanks.
>
>> 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() one checks whether key type is LANDLOCK_KEY_INODE.
>
> is_object_pointer() returns true if the key type is LANDLOCK_KEY_INODE.
>
Got it.
>> get_root() helper chooses red_black tree root depending on a key type.
>
> get_root() returns a red_black tree root pointer according to a key type.
Ok. Thanks.
>
>>
>> Refactors landlock_insert_rule() and landlock_find_rule() to support coming
>
> Refactor…
Ok.
>
>> network modifications. Now adding or searching a rule in a ruleset depends
>
> … Adding or searching a rule in a ruleset can now be done
>
>> on a landlock id argument provided in refactored functions.
>
> thanks to a Landlock ID argument passed to these helpers.
>
I will rewrite it. Thanks.
>>
>> 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 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 | 146 +++++++++++++++++++++++++-----------
>> security/landlock/ruleset.h | 64 +++++++++++++---
>> 3 files changed, 169 insertions(+), 62 deletions(-)
>>
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index 0d57c6479d29..710cfa1306de 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -168,7 +168,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) &&
>> @@ -181,17 +183,17 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>> access_rights |= LANDLOCK_MASK_ACCESS_FS &
>> ~(landlock_get_fs_access_mask(ruleset, 0) |
>> ACCESS_INITIALLY_DENIED);
>> - 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;
>> }
>>
>> @@ -208,6 +210,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))
>> @@ -215,8 +220,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..41de17d1869e 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 inline bool is_object_pointer(const enum landlock_key_type key_type)
>
> Let's remove all "inline" keywords in all .c files. This should be
> simple for the compiler to inline them automatically, and it makes the
> code cleaner.
Got it.
>
>
>> +{
>> + 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 inline struct rb_root *get_root(struct landlock_ruleset *const ruleset,
>
> Please remove "inline".
Got it. Thanks.
>
>> + 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))
>> + return -ENOENT;
>> + }
>> +
>> + root = get_root(ruleset, id.type);
>> + if (IS_ERR(root))
>> + return PTR_ERR(root);
>> +
>> + 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->key.data != id.key.data) {
>> parent_node = *walker_node;
>> - if (this->object < object)
>> + if (this->key.data < id.key.data)
>> walker_node = &((*walker_node)->rb_right);
>> else
>> walker_node = &((*walker_node)->rb_left);
>> @@ -195,24 +238,24 @@ 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,
>> + new_rule = create_rule(id, &this->layers, this->num_layers,
>> &(*layers)[0]);
>> if (IS_ERR(new_rule))
>> return PTR_ERR(new_rule);
>> - rb_replace_node(&this->node, &new_rule->node, &ruleset->root);
>> - free_rule(this);
>> + rb_replace_node(&this->node, &new_rule->node, root);
>> + free_rule(this, id.type);
>> return 0;
>> }
>>
>> - /* There is no match for @object. */
>> + /* There is no match for @id. */
>> build_check_ruleset();
>> if (ruleset->num_rules >= LANDLOCK_MAX_NUM_RULES)
>> return -E2BIG;
>> - new_rule = create_rule(object, layers, num_layers, NULL);
>> + new_rule = create_rule(id, layers, num_layers, NULL);
>> 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, root);
>> ruleset->num_rules++;
>> return 0;
>> }
>> @@ -230,7 +273,7 @@ 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 struct landlock_id id,
>> const access_mask_t access)
>> {
>> struct landlock_layer layers[] = { {
>> @@ -240,7 +283,7 @@ 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, id, &layers, ARRAY_SIZE(layers));
>> }
>>
>> static inline void get_hierarchy(struct landlock_hierarchy *const hierarchy)
>> @@ -263,6 +306,7 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>> struct landlock_ruleset *const src)
>> {
>> struct landlock_rule *walker_rule, *next_rule;
>> + struct rb_root *src_root;
>> int err = 0;
>>
>> might_sleep();
>> @@ -273,6 +317,10 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>> if (WARN_ON_ONCE(!dst || !dst->hierarchy))
>> return -EINVAL;
>>
>> + src_root = get_root(src, LANDLOCK_KEY_INODE);
>> + if (IS_ERR(src_root))
>> + return PTR_ERR(src_root);
>> +
>> /* Locks @dst first because we are its only owner. */
>> mutex_lock(&dst->lock);
>> mutex_lock_nested(&src->lock, SINGLE_DEPTH_NESTING);
>> @@ -285,23 +333,23 @@ 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,
>> + rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, src_root,
>> node) {
>> struct landlock_layer layers[] = { {
>> .level = dst->num_layers,
>> } };
>> + const struct landlock_id id = {
>> + .key = walker_rule->key,
>> + .type = LANDLOCK_KEY_INODE,
>> + };
>>
>> - if (WARN_ON_ONCE(walker_rule->num_layers != 1)) {
>> - err = -EINVAL;
>> - goto out_unlock;
>> - }
>> - if (WARN_ON_ONCE(walker_rule->layers[0].level != 0)) {
>> - err = -EINVAL;
>> - goto out_unlock;
>> - }
>> + if (WARN_ON_ONCE(walker_rule->num_layers != 1))
>> + return -EINVAL;
>> + if (WARN_ON_ONCE(walker_rule->layers[0].level != 0))
>> + return -EINVAL;
>> layers[0].access = walker_rule->layers[0].access;
>> - err = insert_rule(dst, walker_rule->object, &layers,
>> - ARRAY_SIZE(layers));
>> +
>> + err = insert_rule(dst, id, &layers, ARRAY_SIZE(layers));
>> if (err)
>> goto out_unlock;
>> }
>> @@ -316,21 +364,29 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
>> struct landlock_ruleset *const child)
>> {
>> struct landlock_rule *walker_rule, *next_rule;
>> + struct rb_root *parent_root;
>> int err = 0;
>>
>> might_sleep();
>> if (!parent)
>> return 0;
>>
>> + parent_root = get_root(parent, LANDLOCK_KEY_INODE);
>> + if (IS_ERR(parent_root))
>> + return PTR_ERR(parent_root);
>> +
>> /* Locks @child first because we are its only owner. */
>> mutex_lock(&child->lock);
>> mutex_lock_nested(&parent->lock, SINGLE_DEPTH_NESTING);
>>
>> /* Copies the @parent tree. */
>> rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
>> - &parent->root, node) {
>> - err = insert_rule(child, walker_rule->object,
>> - &walker_rule->layers,
>> + parent_root, node) {
>> + const struct landlock_id id = {
>> + .key = walker_rule->key,
>> + .type = LANDLOCK_KEY_INODE,
>> + };
>> + err = insert_rule(child, id, &walker_rule->layers,
>> walker_rule->num_layers);
>> if (err)
>> goto out_unlock;
>> @@ -362,8 +418,9 @@ 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)
>> - free_rule(freeme);
>> + rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root_inode,
>> + node)
>> + free_rule(freeme, LANDLOCK_KEY_INODE);
>> put_hierarchy(ruleset->hierarchy);
>> kfree(ruleset);
>> }
>> @@ -454,20 +511,23 @@ 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 struct landlock_id id)
>> {
>> + const struct rb_root *root;
>> const struct rb_node *node;
>>
>> - if (!object)
>> + root = get_root((struct landlock_ruleset *)ruleset, id.type);
>> + if (IS_ERR(root))
>> return NULL;
>> - node = ruleset->root.rb_node;
>> + node = root->rb_node;
>> +
>> while (node) {
>> struct landlock_rule *this =
>> rb_entry(node, struct landlock_rule, node);
>>
>> - if (this->object == object)
>> + if (this->key.data == id.key.data)
>> return this;
>> - if (this->object < object)
>> + if (this->key.data < id.key.data)
>> node = node->rb_right;
>> else
>> node = node->rb_left;
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index f2ad932d396c..608ab356bc3e 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -49,6 +49,46 @@ struct landlock_layer {
>> access_mask_t access;
>> };
>>
>> +/**
>> + * union landlock_key - Key of a ruleset's red-black tree
>> + */
>> +union landlock_key {
>> + /**
>> + * @object: Pointer to identify a kernel object (e.g. an inode).
>> + */
>> + struct landlock_object *object;
>> + /**
>> + * @data: A raw data value to identify a network socket port.
>
> "Raw data to identify an arbitrary 32-bit value (e.g. a TCP port)."
>
Ok.
>
>> + */
>> + uintptr_t data;
>> +};
>> +
>> +/**
>> + * enum landlock_key_type - Type of &union landlock_key
>> + */
>> +enum landlock_key_type {
>> + /**
>> + * @LANDLOCK_KEY_INODE: Type of &landlock_ruleset.root_inode's node
>> + * keys.
>> + */
>> + LANDLOCK_KEY_INODE = 1,
>> +};
>> +
>> +/**
>> + * struct landlock_id - Unique rule identifier for a ruleset
>> + */
>> +struct landlock_id {
>> + /**
>> + * @key: A union to identify either a kernel object (e.g. an inode) or
>> + * a raw data value (e.g. a network socket port).
>
> "a 32-bit value (e.g. a TCP port)."
Ok. Got it.
>
>
>> + */
>> + union landlock_key key;
>> + /**
>> + * @type: A enumerator to identify the type of landlock_ruleset's root tree.
>> + */
>> + const enum landlock_key_type type;
>> +};
>> +
>> /**
>> * struct landlock_rule - Access rights tied to an object
>> */
>> @@ -58,12 +98,13 @@ 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.
>> + * @key: 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. The 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;
>> + union landlock_key key;
>> /**
>> * @num_layers: Number of entries in @layers.
>> */
>> @@ -99,11 +140,12 @@ struct landlock_hierarchy {
>> */
>> struct landlock_ruleset {
>> /**
>> - * @root: Root of a red-black tree containing &struct landlock_rule
>> - * nodes. Once a ruleset is tied to a process (i.e. as a domain), this
>> - * tree is immutable until @usage reaches zero.
>> + * @root_inode: Root of a red-black tree containing &struct
>> + * landlock_rule nodes with inode object. Once a ruleset is tied to a
>> + * process (i.e. as a domain), this tree is immutable until @usage
>> + * reaches zero.
>> */
>> - struct rb_root root;
>> + struct rb_root root_inode;
>> /**
>> * @hierarchy: Enables hierarchy identification even when a parent
>> * domain vanishes. This is needed for the ptrace protection.
>> @@ -165,7 +207,7 @@ 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 struct landlock_id id,
>> const access_mask_t access);
>>
>> struct landlock_ruleset *
>> @@ -174,7 +216,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 struct landlock_id id);
>>
>> static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
>> {
>> --
>> 2.25.1
>>
> .
Powered by blists - more mailing lists