| Message ID | 20220309134459.6448-4-konstantin.meskhidze@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Landlock LSM | expand |
On 09/03/2022 14:44, Konstantin Meskhidze wrote: > A new object union added to support a socket port > rule type. To support it landlock_insert_rule() and > landlock_find_rule() were refactored. 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 v3: > * Split commit. > * Refactoring landlock_insert_rule and landlock_find_rule functions. > * Rename new_ruleset->root_inode. > > --- > security/landlock/fs.c | 5 +- > security/landlock/ruleset.c | 108 +++++++++++++++++++++++++----------- > security/landlock/ruleset.h | 26 +++++---- > 3 files changed, 94 insertions(+), 45 deletions(-) > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > index 97f5c455f5a7..1497948d754f 100644 > --- a/security/landlock/fs.c > +++ b/security/landlock/fs.c > @@ -168,7 +168,7 @@ 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); For consistency, please use 80 columns everywhere. > mutex_unlock(&ruleset->lock); > /* > * No need to check for an error because landlock_insert_rule() > @@ -195,7 +195,8 @@ static inline u64 unmask_layers( > inode = d_backing_inode(path->dentry); > rcu_read_lock(); > rule = landlock_find_rule(domain, > - rcu_dereference(landlock_inode(inode)->object)); > + (uintptr_t)rcu_dereference(landlock_inode(inode)->object), > + LANDLOCK_RULE_PATH_BENEATH); > rcu_read_unlock(); > if (!rule) > return layer_mask; > diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c > index a6212b752549..971685c48641 100644 > --- a/security/landlock/ruleset.c > +++ b/security/landlock/ruleset.c > @@ -34,7 +34,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 > @@ -81,10 +81,12 @@ static void build_check_rule(void) > } > > static struct landlock_rule *create_rule( > - struct landlock_object *const object, > + 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) > + const struct landlock_layer *const new_layer, > + const u16 rule_type) > { > struct landlock_rule *new_rule; > u32 new_num_layers; > @@ -103,8 +105,16 @@ static struct landlock_rule *create_rule( > if (!new_rule) > return ERR_PTR(-ENOMEM); > RB_CLEAR_NODE(&new_rule->node); > - landlock_get_object(object); > - new_rule->object = object; > + > + switch (rule_type) { > + case LANDLOCK_RULE_PATH_BENEATH: > + landlock_get_object(object_ptr); > + new_rule->object.ptr = object_ptr; > + break; > + default: > + return ERR_PTR(-EINVAL); This would lead to memory leak. You should at least add a WARN_ON_ONCE(1) here, but a proper solution would be to remove the use of rule_type and only rely on object_ptr and object_data values. You can also add a WARN_ON_ONCE(object_ptr && object_data). > + } > + > new_rule->num_layers = new_num_layers; > /* Copies the original layer stack. */ > memcpy(new_rule->layers, layers, > @@ -120,7 +130,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); > } > > @@ -156,26 +166,38 @@ 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, > + const uintptr_t object_data, > const struct landlock_layer (*const layers)[], > - size_t num_layers) > + size_t num_layers, u16 rule_type) > { > struct rb_node **walker_node; > struct rb_node *parent_node = NULL; > struct landlock_rule *new_rule; > + uintptr_t object; > + struct rb_root *root; > > might_sleep(); > lockdep_assert_held(&ruleset->lock); > - if (WARN_ON_ONCE(!object || !layers)) > - return -ENOENT; You can leave this code here. > - walker_node = &(ruleset->root.rb_node); > + /* Choose rb_tree structure depending on a rule type */ > + switch (rule_type) { > + case LANDLOCK_RULE_PATH_BENEATH: > + if (WARN_ON_ONCE(!object_ptr || !layers)) > + return -ENOENT; > + object = (uintptr_t)object_ptr; > + root = &ruleset->root_inode; > + break; > + default: > + 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) { > parent_node = *walker_node; > - if (this->object < object) > + if (this->object.data < object) > walker_node = &((*walker_node)->rb_right); > else > walker_node = &((*walker_node)->rb_left); > @@ -207,11 +229,15 @@ 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: Same here and for the following code, you should replace such switch/case with an if (object_ptr). > + new_rule = create_rule(object_ptr, 0, &this->layers, this->num_layers, > + &(*layers)[0], rule_type); > + 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); Use the root variable here. Same for the following code and patches. > free_rule(this); > return 0; > } > @@ -220,11 +246,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, rule_type); > + 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; > } > @@ -242,7 +272,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 u32 access) > + struct landlock_object *const object_ptr, > + const uintptr_t object_data, > + const u32 access, const u16 rule_type) > { > struct landlock_layer layers[] = {{ > .access = access, > @@ -251,7 +283,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, &layers, > + ARRAY_SIZE(layers), rule_type); > } > > static inline void get_hierarchy(struct landlock_hierarchy *const hierarchy) > @@ -297,7 +330,7 @@ static int merge_ruleset(struct landlock_ruleset *const dst, > > /* Merges the @src tree. */ > rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, > - &src->root, node) { > + &src->root_inode, node) { > struct landlock_layer layers[] = {{ > .level = dst->num_layers, > }}; > @@ -311,8 +344,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, > - ARRAY_SIZE(layers)); > + err = insert_rule(dst, walker_rule->object.ptr, 0, &layers, > + ARRAY_SIZE(layers), LANDLOCK_RULE_PATH_BENEATH); > if (err) > goto out_unlock; > } > @@ -323,6 +356,8 @@ static int merge_ruleset(struct landlock_ruleset *const dst, > return err; > } > > + > + Useless lines. > static int inherit_ruleset(struct landlock_ruleset *const parent, > struct landlock_ruleset *const child) > { > @@ -339,9 +374,10 @@ 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, > - &walker_rule->layers, walker_rule->num_layers); > + &parent->root_inode, node) { > + err = insert_rule(child, walker_rule->object.ptr, 0, > + &walker_rule->layers, walker_rule->num_layers, > + LANDLOCK_RULE_PATH_BENEATH); > if (err) > goto out_unlock; > } > @@ -372,7 +408,7 @@ 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, > + rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root_inode, > node) > free_rule(freeme); > put_hierarchy(ruleset->hierarchy); > @@ -465,20 +501,28 @@ struct landlock_ruleset *landlock_merge_ruleset( > */ > 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) object_data can be 0. You need to add a test with such value. We need to be sure that this change cannot affect the current FS code. > return NULL; > - node = ruleset->root.rb_node; > + > + switch (rule_type) { > + case LANDLOCK_RULE_PATH_BENEATH: > + node = ruleset->root_inode.rb_node; > + break; > + default: > + return ERR_PTR(-EINVAL); This is a bug. There is no check for such value. You need to check and update all call sites to catch such errors. Same for all new use of ERR_PTR(). > + } > + > 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 bc87e5f787f7..088b8d95f653 100644 > --- a/security/landlock/ruleset.h > +++ b/security/landlock/ruleset.h > @@ -50,15 +50,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 socket port object. …or a raw data value (e.g. a network socket port). > This is used as a key for this ruleset element. > + * This pointer is set once and never modified. It always points to an s/This pointer/@object.ptr/ > + * 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 > @@ -95,7 +97,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. > @@ -157,7 +159,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 u32 access); > + struct landlock_object *const object_ptr, > + const uintptr_t object_data, > + const u32 access, const u16 rule_type); > > struct landlock_ruleset *landlock_merge_ruleset( > struct landlock_ruleset *const parent, > @@ -165,7 +169,7 @@ struct landlock_ruleset *landlock_merge_ruleset( > > 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 >
3/16/2022 11:27 AM, Mickaël Salaün пишет: > > On 09/03/2022 14:44, Konstantin Meskhidze wrote: >> A new object union added to support a socket port >> rule type. To support it landlock_insert_rule() and >> landlock_find_rule() were refactored. 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 v3: >> * Split commit. >> * Refactoring landlock_insert_rule and landlock_find_rule functions. >> * Rename new_ruleset->root_inode. >> >> --- >> security/landlock/fs.c | 5 +- >> security/landlock/ruleset.c | 108 +++++++++++++++++++++++++----------- >> security/landlock/ruleset.h | 26 +++++---- >> 3 files changed, 94 insertions(+), 45 deletions(-) >> >> diff --git a/security/landlock/fs.c b/security/landlock/fs.c >> index 97f5c455f5a7..1497948d754f 100644 >> --- a/security/landlock/fs.c >> +++ b/security/landlock/fs.c >> @@ -168,7 +168,7 @@ 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); > > For consistency, please use 80 columns everywhere. Ok. I got it. > >> mutex_unlock(&ruleset->lock); >> /* >> * No need to check for an error because landlock_insert_rule() >> @@ -195,7 +195,8 @@ static inline u64 unmask_layers( >> inode = d_backing_inode(path->dentry); >> rcu_read_lock(); >> rule = landlock_find_rule(domain, >> - rcu_dereference(landlock_inode(inode)->object)); >> + (uintptr_t)rcu_dereference(landlock_inode(inode)->object), >> + LANDLOCK_RULE_PATH_BENEATH); >> rcu_read_unlock(); >> if (!rule) >> return layer_mask; >> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c >> index a6212b752549..971685c48641 100644 >> --- a/security/landlock/ruleset.c >> +++ b/security/landlock/ruleset.c >> @@ -34,7 +34,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 >> @@ -81,10 +81,12 @@ static void build_check_rule(void) >> } >> >> static struct landlock_rule *create_rule( >> - struct landlock_object *const object, >> + 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) >> + const struct landlock_layer *const new_layer, >> + const u16 rule_type) >> { >> struct landlock_rule *new_rule; >> u32 new_num_layers; >> @@ -103,8 +105,16 @@ static struct landlock_rule *create_rule( >> if (!new_rule) >> return ERR_PTR(-ENOMEM); >> RB_CLEAR_NODE(&new_rule->node); >> - landlock_get_object(object); >> - new_rule->object = object; >> + >> + switch (rule_type) { >> + case LANDLOCK_RULE_PATH_BENEATH: >> + landlock_get_object(object_ptr); >> + new_rule->object.ptr = object_ptr; >> + break; >> + default: >> + return ERR_PTR(-EINVAL); > > This would lead to memory leak. You should at least add a > WARN_ON_ONCE(1) here, but a proper solution would be to remove the use > of rule_type and only rely on object_ptr and object_data values. You can > also add a WARN_ON_ONCE(object_ptr && object_data). > > But rule_type is needed here in coming commits to support network rules. For LANDLOCK_RULE_PATH_BENEATH rule type landlock_get_object() is used but for LANDLOCK_RULE_NET_SERVICE is not. Using rule type is convenient for distinguising between fs and network rules. >> + } >> + >> new_rule->num_layers = new_num_layers; >> /* Copies the original layer stack. */ >> memcpy(new_rule->layers, layers, >> @@ -120,7 +130,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); >> } >> >> @@ -156,26 +166,38 @@ 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, >> + const uintptr_t object_data, >> const struct landlock_layer (*const layers)[], >> - size_t num_layers) >> + size_t num_layers, u16 rule_type) >> { >> struct rb_node **walker_node; >> struct rb_node *parent_node = NULL; >> struct landlock_rule *new_rule; >> + uintptr_t object; >> + struct rb_root *root; >> >> might_sleep(); >> lockdep_assert_held(&ruleset->lock); >> - if (WARN_ON_ONCE(!object || !layers)) >> - return -ENOENT; > > You can leave this code here. But anyway in coming commits with network rules this code will be moved into case LANDLOCK_RULE_PATH_BENEATH: .... > >> - walker_node = &(ruleset->root.rb_node); >> + /* Choose rb_tree structure depending on a rule type */ >> + switch (rule_type) { >> + case LANDLOCK_RULE_PATH_BENEATH: >> + if (WARN_ON_ONCE(!object_ptr || !layers)) >> + return -ENOENT; >> + object = (uintptr_t)object_ptr; >> + root = &ruleset->root_inode; >> + break; >> + default: >> + 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) { >> parent_node = *walker_node; >> - if (this->object < object) >> + if (this->object.data < object) >> walker_node = &((*walker_node)->rb_right); >> else >> walker_node = &((*walker_node)->rb_left); >> @@ -207,11 +229,15 @@ 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: > > Same here and for the following code, you should replace such > switch/case with an if (object_ptr). > What about coming commits with network rule_type support? > >> + new_rule = create_rule(object_ptr, 0, &this->layers, >> this->num_layers, >> + &(*layers)[0], rule_type); >> + 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); > > Use the root variable here. Same for the following code and patches. What about your suggestion to use 2 rb_tress to support different rule_types: 1. root_inode - for filesystem objects 2. root_net_port - for network port objects ???? > > >> free_rule(this); >> return 0; >> } >> @@ -220,11 +246,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, rule_type); >> + 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; >> } >> @@ -242,7 +272,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 u32 access) >> + struct landlock_object *const object_ptr, >> + const uintptr_t object_data, >> + const u32 access, const u16 rule_type) >> { >> struct landlock_layer layers[] = {{ >> .access = access, >> @@ -251,7 +283,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, &layers, >> + ARRAY_SIZE(layers), rule_type); >> } >> >> static inline void get_hierarchy(struct landlock_hierarchy *const >> hierarchy) >> @@ -297,7 +330,7 @@ static int merge_ruleset(struct landlock_ruleset >> *const dst, >> >> /* Merges the @src tree. */ >> rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, >> - &src->root, node) { >> + &src->root_inode, node) { >> struct landlock_layer layers[] = {{ >> .level = dst->num_layers, >> }}; >> @@ -311,8 +344,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, >> - ARRAY_SIZE(layers)); >> + err = insert_rule(dst, walker_rule->object.ptr, 0, &layers, >> + ARRAY_SIZE(layers), LANDLOCK_RULE_PATH_BENEATH); >> if (err) >> goto out_unlock; >> } >> @@ -323,6 +356,8 @@ static int merge_ruleset(struct landlock_ruleset >> *const dst, >> return err; >> } >> >> + >> + > > Useless lines. Got it. Thanks. > > >> static int inherit_ruleset(struct landlock_ruleset *const parent, >> struct landlock_ruleset *const child) >> { >> @@ -339,9 +374,10 @@ 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, >> - &walker_rule->layers, walker_rule->num_layers); >> + &parent->root_inode, node) { >> + err = insert_rule(child, walker_rule->object.ptr, 0, >> + &walker_rule->layers, walker_rule->num_layers, >> + LANDLOCK_RULE_PATH_BENEATH); >> if (err) >> goto out_unlock; >> } >> @@ -372,7 +408,7 @@ 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, >> + rbtree_postorder_for_each_entry_safe(freeme, next, >> &ruleset->root_inode, >> node) >> free_rule(freeme); >> put_hierarchy(ruleset->hierarchy); >> @@ -465,20 +501,28 @@ struct landlock_ruleset *landlock_merge_ruleset( >> */ >> 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) > > object_data can be 0. You need to add a test with such value. > > We need to be sure that this change cannot affect the current FS code. I got it. I will refactor it. > > >> return NULL; >> - node = ruleset->root.rb_node; >> + >> + switch (rule_type) { >> + case LANDLOCK_RULE_PATH_BENEATH: >> + node = ruleset->root_inode.rb_node; >> + break; >> + default: >> + return ERR_PTR(-EINVAL); > > This is a bug. There is no check for such value. You need to check and > update all call sites to catch such errors. Same for all new use of > ERR_PTR(). Sorry, I did not get your point. Do you mean I should check the correctness of rule_type in above function which calls landlock_find_rule() ??? Why can't I add such check here? > > >> + } >> + >> 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 bc87e5f787f7..088b8d95f653 100644 >> --- a/security/landlock/ruleset.h >> +++ b/security/landlock/ruleset.h >> @@ -50,15 +50,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 socket port object. > > …or a raw data value (e.g. a network socket port). > Ok. I will mofdify this line > >> This is used as a key for this ruleset element. >> + * This pointer is set once and never modified. It always points >> to an > > s/This pointer/@object.ptr/ Ok. I got it. > > >> + * 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 >> @@ -95,7 +97,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. >> @@ -157,7 +159,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 u32 access); >> + struct landlock_object *const object_ptr, >> + const uintptr_t object_data, >> + const u32 access, const u16 rule_type); >> >> struct landlock_ruleset *landlock_merge_ruleset( >> struct landlock_ruleset *const parent, >> @@ -165,7 +169,7 @@ struct landlock_ruleset *landlock_merge_ruleset( >> >> 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 >> > .
On 17/03/2022 15:29, Konstantin Meskhidze wrote: > > > 3/16/2022 11:27 AM, Mickaël Salaün пишет: >> >> On 09/03/2022 14:44, Konstantin Meskhidze wrote: >>> A new object union added to support a socket port >>> rule type. To support it landlock_insert_rule() and >>> landlock_find_rule() were refactored. 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 v3: >>> * Split commit. >>> * Refactoring landlock_insert_rule and landlock_find_rule functions. >>> * Rename new_ruleset->root_inode. >>> >>> --- >>> security/landlock/fs.c | 5 +- >>> security/landlock/ruleset.c | 108 +++++++++++++++++++++++++----------- >>> security/landlock/ruleset.h | 26 +++++---- >>> 3 files changed, 94 insertions(+), 45 deletions(-) >>> >>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c >>> index 97f5c455f5a7..1497948d754f 100644 >>> --- a/security/landlock/fs.c >>> +++ b/security/landlock/fs.c >>> @@ -168,7 +168,7 @@ 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); >> >> For consistency, please use 80 columns everywhere. > > Ok. I got it. >> >>> mutex_unlock(&ruleset->lock); >>> /* >>> * No need to check for an error because landlock_insert_rule() >>> @@ -195,7 +195,8 @@ static inline u64 unmask_layers( >>> inode = d_backing_inode(path->dentry); >>> rcu_read_lock(); >>> rule = landlock_find_rule(domain, >>> - rcu_dereference(landlock_inode(inode)->object)); >>> + (uintptr_t)rcu_dereference(landlock_inode(inode)->object), >>> + LANDLOCK_RULE_PATH_BENEATH); >>> rcu_read_unlock(); >>> if (!rule) >>> return layer_mask; >>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c >>> index a6212b752549..971685c48641 100644 >>> --- a/security/landlock/ruleset.c >>> +++ b/security/landlock/ruleset.c >>> @@ -34,7 +34,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 >>> @@ -81,10 +81,12 @@ static void build_check_rule(void) >>> } >>> >>> static struct landlock_rule *create_rule( >>> - struct landlock_object *const object, >>> + 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) >>> + const struct landlock_layer *const new_layer, >>> + const u16 rule_type) >>> { >>> struct landlock_rule *new_rule; >>> u32 new_num_layers; >>> @@ -103,8 +105,16 @@ static struct landlock_rule *create_rule( >>> if (!new_rule) >>> return ERR_PTR(-ENOMEM); >>> RB_CLEAR_NODE(&new_rule->node); >>> - landlock_get_object(object); >>> - new_rule->object = object; >>> + >>> + switch (rule_type) { >>> + case LANDLOCK_RULE_PATH_BENEATH: >>> + landlock_get_object(object_ptr); >>> + new_rule->object.ptr = object_ptr; >>> + break; >>> + default: >>> + return ERR_PTR(-EINVAL); >> >> This would lead to memory leak. You should at least add a >> WARN_ON_ONCE(1) here, but a proper solution would be to remove the use >> of rule_type and only rely on object_ptr and object_data values. You >> can also add a WARN_ON_ONCE(object_ptr && object_data). >> >> > But rule_type is needed here in coming commits to support network > rules. For LANDLOCK_RULE_PATH_BENEATH rule type landlock_get_object() > is used but for LANDLOCK_RULE_NET_SERVICE is not. Using rule type is > convenient for distinguising between fs and network rules. rule_type is not required to infer if the rule use a pointer or raw data, even with the following commits, because you can rely on object_ptr being NULL or not. This would make create_rule() generic for pointer-based and data-based object, even if not-yet-existing rule types. It is less error-prone to only be able to infer something from one source (i.e. object_ptr and not rule_type). >>> + } >>> + >>> new_rule->num_layers = new_num_layers; >>> /* Copies the original layer stack. */ >>> memcpy(new_rule->layers, layers, >>> @@ -120,7 +130,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); >>> } >>> >>> @@ -156,26 +166,38 @@ 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, >>> + const uintptr_t object_data, Can you move rule_type here for this function and similar ones? It makes sense to group object-related arguments. >>> const struct landlock_layer (*const layers)[], >>> - size_t num_layers) >>> + size_t num_layers, u16 rule_type) >>> { >>> struct rb_node **walker_node; >>> struct rb_node *parent_node = NULL; >>> struct landlock_rule *new_rule; >>> + uintptr_t object; >>> + struct rb_root *root; >>> >>> might_sleep(); >>> lockdep_assert_held(&ruleset->lock); >>> - if (WARN_ON_ONCE(!object || !layers)) >>> - return -ENOENT; >> >> You can leave this code here. > > But anyway in coming commits with network rules this code will be > moved into case LANDLOCK_RULE_PATH_BENEATH: .... Yes, but without rule_type you don't need to duplicate this check, just to remove object_ptr from WARN_ON_ONCE() and replace the rule_type switch/case with if (object_ptr). You can change to this: --- a/security/landlock/ruleset.c +++ b/security/landlock/ruleset.c @@ -194,43 +194,49 @@ static void build_check_ruleset(void) */ static int insert_rule(struct landlock_ruleset *const ruleset, struct landlock_object *const object_ptr, - const uintptr_t object_data, + uintptr_t object_data, /* move @rule_type here */ const struct landlock_layer (*const layers)[], - size_t num_layers, u16 rule_type) + size_t num_layers, const enum landlock_rule_type rule_type) { struct rb_node **walker_node; struct rb_node *parent_node = NULL; struct landlock_rule *new_rule; - uintptr_t object; struct rb_root *root; might_sleep(); lockdep_assert_held(&ruleset->lock); - /* Choose rb_tree structure depending on a rule type */ + + if (WARN_ON_ONCE(!layers)) + return -ENOENT; + if (WARN_ON_ONCE(object_ptr && object_data)) + return -EINVAL; + + /* Chooses the rb_tree according to the rule type. */ switch (rule_type) { case LANDLOCK_RULE_PATH_BENEATH: - if (WARN_ON_ONCE(!object_ptr || !layers)) + if (WARN_ON_ONCE(!object_ptr)) return -ENOENT; - object = (uintptr_t)object_ptr; + object_data = (uintptr_t)object_ptr; root = &ruleset->root_inode; break; case LANDLOCK_RULE_NET_SERVICE: - if (WARN_ON_ONCE(!object_data || !layers)) - return -ENOENT; - object = object_data; + if (WARN_ON_ONCE(object_ptr)) + return -EINVAL; root = &ruleset->root_net_port; 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.data != object) { + if (this->object.data != object_data) { parent_node = *walker_node; - if (this->object.data < object) + if (this->object.data < object_data) walker_node = &((*walker_node)->rb_right); else walker_node = &((*walker_node)->rb_left); This highlight an implicit error handling for a port value of 0. I'm not sure if this should be allowed or not though. If not, it should be an explicit service_port check in add_rule_net_service(). A data value of zero might be legitimate for this use case or not-yet-existing data-based rule types. Anyway, this kind of check is specific to the use case and should not be part of insert_rule(). >> >>> - walker_node = &(ruleset->root.rb_node); >>> + /* Choose rb_tree structure depending on a rule type */ >>> + switch (rule_type) { >>> + case LANDLOCK_RULE_PATH_BENEATH: >>> + if (WARN_ON_ONCE(!object_ptr || !layers)) >>> + return -ENOENT; >>> + object = (uintptr_t)object_ptr; >>> + root = &ruleset->root_inode; >>> + break; >>> + default: >>> + 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) { >>> parent_node = *walker_node; >>> - if (this->object < object) >>> + if (this->object.data < object) >>> walker_node = &((*walker_node)->rb_right); >>> else >>> walker_node = &((*walker_node)->rb_left); >>> @@ -207,11 +229,15 @@ 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: >> >> Same here and for the following code, you should replace such >> switch/case with an if (object_ptr). >> What about coming commits with network rule_type support? This will still works. >> >>> + new_rule = create_rule(object_ptr, 0, &this->layers, >>> this->num_layers, >>> + &(*layers)[0], rule_type); >>> + 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); >> >> Use the root variable here. Same for the following code and patches. > > What about your suggestion to use 2 rb_tress to support different > rule_types: > 1. root_inode - for filesystem objects > 2. root_net_port - for network port objects > ???? I was talking about the root variable you declared a few line before. The conversion from ruleset->root to ruleset->root_inode is fine. [...] >>> @@ -465,20 +501,28 @@ struct landlock_ruleset *landlock_merge_ruleset( >>> */ >>> 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) >> >> object_data can be 0. You need to add a test with such value. >> >> We need to be sure that this change cannot affect the current FS code. > > I got it. I will refactor it. Well, 0 means a port 0, which might not be correct, but this check should not be performed by landlock_merge_ruleset(). >> >> >>> return NULL; >>> - node = ruleset->root.rb_node; >>> + >>> + switch (rule_type) { >>> + case LANDLOCK_RULE_PATH_BENEATH: >>> + node = ruleset->root_inode.rb_node; >>> + break; >>> + default: >>> + return ERR_PTR(-EINVAL); >> >> This is a bug. There is no check for such value. You need to check and >> update all call sites to catch such errors. Same for all new use of >> ERR_PTR(). > > Sorry, I did not get your point. > Do you mean I should check the correctness of rule_type in above > function which calls landlock_find_rule() ??? Why can't I add such check > here? landlock_find_rule() only returns NULL or a valid pointer, not an error. [...]
3/18/2022 9:33 PM, Mickaël Salaün пишет: > > On 17/03/2022 15:29, Konstantin Meskhidze wrote: >> >> >> 3/16/2022 11:27 AM, Mickaël Salaün пишет: >>> >>> On 09/03/2022 14:44, Konstantin Meskhidze wrote: >>>> A new object union added to support a socket port >>>> rule type. To support it landlock_insert_rule() and >>>> landlock_find_rule() were refactored. 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 v3: >>>> * Split commit. >>>> * Refactoring landlock_insert_rule and landlock_find_rule functions. >>>> * Rename new_ruleset->root_inode. >>>> >>>> --- >>>> security/landlock/fs.c | 5 +- >>>> security/landlock/ruleset.c | 108 >>>> +++++++++++++++++++++++++----------- >>>> security/landlock/ruleset.h | 26 +++++---- >>>> 3 files changed, 94 insertions(+), 45 deletions(-) >>>> >>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c >>>> index 97f5c455f5a7..1497948d754f 100644 >>>> --- a/security/landlock/fs.c >>>> +++ b/security/landlock/fs.c >>>> @@ -168,7 +168,7 @@ 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); >>> >>> For consistency, please use 80 columns everywhere. >> >> Ok. I got it. >>> >>>> mutex_unlock(&ruleset->lock); >>>> /* >>>> * No need to check for an error because landlock_insert_rule() >>>> @@ -195,7 +195,8 @@ static inline u64 unmask_layers( >>>> inode = d_backing_inode(path->dentry); >>>> rcu_read_lock(); >>>> rule = landlock_find_rule(domain, >>>> - rcu_dereference(landlock_inode(inode)->object)); >>>> + (uintptr_t)rcu_dereference(landlock_inode(inode)->object), >>>> + LANDLOCK_RULE_PATH_BENEATH); >>>> rcu_read_unlock(); >>>> if (!rule) >>>> return layer_mask; >>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c >>>> index a6212b752549..971685c48641 100644 >>>> --- a/security/landlock/ruleset.c >>>> +++ b/security/landlock/ruleset.c >>>> @@ -34,7 +34,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 >>>> @@ -81,10 +81,12 @@ static void build_check_rule(void) >>>> } >>>> >>>> static struct landlock_rule *create_rule( >>>> - struct landlock_object *const object, >>>> + 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) >>>> + const struct landlock_layer *const new_layer, >>>> + const u16 rule_type) >>>> { >>>> struct landlock_rule *new_rule; >>>> u32 new_num_layers; >>>> @@ -103,8 +105,16 @@ static struct landlock_rule *create_rule( >>>> if (!new_rule) >>>> return ERR_PTR(-ENOMEM); >>>> RB_CLEAR_NODE(&new_rule->node); >>>> - landlock_get_object(object); >>>> - new_rule->object = object; >>>> + >>>> + switch (rule_type) { >>>> + case LANDLOCK_RULE_PATH_BENEATH: >>>> + landlock_get_object(object_ptr); >>>> + new_rule->object.ptr = object_ptr; >>>> + break; >>>> + default: >>>> + return ERR_PTR(-EINVAL); >>> >>> This would lead to memory leak. You should at least add a >>> WARN_ON_ONCE(1) here, but a proper solution would be to remove the >>> use of rule_type and only rely on object_ptr and object_data values. >>> You can also add a WARN_ON_ONCE(object_ptr && object_data). >>> >>> >> But rule_type is needed here in coming commits to support network >> rules. For LANDLOCK_RULE_PATH_BENEATH rule type >> landlock_get_object() is used but for LANDLOCK_RULE_NET_SERVICE is >> not. Using rule type is convenient for distinguising between fs and >> network rules. > > rule_type is not required to infer if the rule use a pointer or raw > data, even with the following commits, because you can rely on > object_ptr being NULL or not. This would make create_rule() generic for > pointer-based and data-based object, even if not-yet-existing rule > types. It is less error-prone to only be able to infer something from > one source (i.e. object_ptr and not rule_type). > Ok. I got you. Will be refactored. > >>>> + } >>>> + >>>> new_rule->num_layers = new_num_layers; >>>> /* Copies the original layer stack. */ >>>> memcpy(new_rule->layers, layers, >>>> @@ -120,7 +130,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); >>>> } >>>> >>>> @@ -156,26 +166,38 @@ 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, >>>> + const uintptr_t object_data, > > Can you move rule_type here for this function and similar ones? It makes > sense to group object-related arguments. Just to group them together, not putting rule_type in the end? > > >>>> const struct landlock_layer (*const layers)[], >>>> - size_t num_layers) >>>> + size_t num_layers, u16 rule_type) >>>> { >>>> struct rb_node **walker_node; >>>> struct rb_node *parent_node = NULL; >>>> struct landlock_rule *new_rule; >>>> + uintptr_t object; >>>> + struct rb_root *root; >>>> >>>> might_sleep(); >>>> lockdep_assert_held(&ruleset->lock); >>>> - if (WARN_ON_ONCE(!object || !layers)) >>>> - return -ENOENT; >>> >>> You can leave this code here. >> >> But anyway in coming commits with network rules this code will be >> moved into case LANDLOCK_RULE_PATH_BENEATH: .... > > Yes, but without rule_type you don't need to duplicate this check, just > to remove object_ptr from WARN_ON_ONCE() and replace the rule_type > switch/case with if (object_ptr). > > You can change to this: > > --- a/security/landlock/ruleset.c > +++ b/security/landlock/ruleset.c > @@ -194,43 +194,49 @@ static void build_check_ruleset(void) > */ > static int insert_rule(struct landlock_ruleset *const ruleset, > struct landlock_object *const object_ptr, > - const uintptr_t object_data, > + uintptr_t object_data, /* move @rule_type here */ > const struct landlock_layer (*const layers)[], > - size_t num_layers, u16 rule_type) > + size_t num_layers, const enum landlock_rule_type rule_type) > { > struct rb_node **walker_node; > struct rb_node *parent_node = NULL; > struct landlock_rule *new_rule; > - uintptr_t object; > struct rb_root *root; > > might_sleep(); > lockdep_assert_held(&ruleset->lock); > - /* Choose rb_tree structure depending on a rule type */ > + > + if (WARN_ON_ONCE(!layers)) > + return -ENOENT; > + if (WARN_ON_ONCE(object_ptr && object_data)) > + return -EINVAL; > + > + /* Chooses the rb_tree according to the rule type. */ > switch (rule_type) { > case LANDLOCK_RULE_PATH_BENEATH: > - if (WARN_ON_ONCE(!object_ptr || !layers)) > + if (WARN_ON_ONCE(!object_ptr)) > return -ENOENT; > - object = (uintptr_t)object_ptr; > + object_data = (uintptr_t)object_ptr; > root = &ruleset->root_inode; > break; > case LANDLOCK_RULE_NET_SERVICE: > - if (WARN_ON_ONCE(!object_data || !layers)) > - return -ENOENT; > - object = object_data; > + if (WARN_ON_ONCE(object_ptr)) > + return -EINVAL; > root = &ruleset->root_net_port; > 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.data != object) { > + if (this->object.data != object_data) { > parent_node = *walker_node; > - if (this->object.data < object) > + if (this->object.data < object_data) > walker_node = &((*walker_node)->rb_right); > else > walker_node = &((*walker_node)->rb_left); > > > This highlight an implicit error handling for a port value of 0. I'm not > sure if this should be allowed or not though. If not, it should be an > explicit service_port check in add_rule_net_service(). A data value of > zero might be legitimate for this use case or not-yet-existing > data-based rule types. Anyway, this kind of check is specific to the use > case and should not be part of insert_rule(). > Ok. I got it. > > >>> >>>> - walker_node = &(ruleset->root.rb_node); >>>> + /* Choose rb_tree structure depending on a rule type */ >>>> + switch (rule_type) { >>>> + case LANDLOCK_RULE_PATH_BENEATH: >>>> + if (WARN_ON_ONCE(!object_ptr || !layers)) >>>> + return -ENOENT; >>>> + object = (uintptr_t)object_ptr; >>>> + root = &ruleset->root_inode; >>>> + break; >>>> + default: >>>> + 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) { >>>> parent_node = *walker_node; >>>> - if (this->object < object) >>>> + if (this->object.data < object) >>>> walker_node = &((*walker_node)->rb_right); >>>> else >>>> walker_node = &((*walker_node)->rb_left); >>>> @@ -207,11 +229,15 @@ 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: >>> >>> Same here and for the following code, you should replace such >>> switch/case with an if (object_ptr). >>> What about coming commits with network rule_type support? > > This will still works. > Yep. Ok. > >>> >>>> + new_rule = create_rule(object_ptr, 0, &this->layers, >>>> this->num_layers, >>>> + &(*layers)[0], rule_type); >>>> + 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); >>> >>> Use the root variable here. Same for the following code and patches. >> >> What about your suggestion to use 2 rb_tress to support different >> rule_types: >> 1. root_inode - for filesystem objects >> 2. root_net_port - for network port objects >> ???? > > I was talking about the root variable you declared a few line before. > The conversion from ruleset->root to ruleset->root_inode is fine. > Sorry. It was a misunderstanding. Got your point. > > [...] > >>>> @@ -465,20 +501,28 @@ struct landlock_ruleset *landlock_merge_ruleset( >>>> */ >>>> 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) >>> >>> object_data can be 0. You need to add a test with such value. >>> >>> We need to be sure that this change cannot affect the current FS code. >> >> I got it. I will refactor it. > > Well, 0 means a port 0, which might not be correct, but this check > should not be performed by landlock_merge_ruleset(). > Do you mean landlock_find_rule()?? Cause this check is not performed in landlock_merge_ruleset(). > >>> >>> >>>> return NULL; >>>> - node = ruleset->root.rb_node; >>>> + >>>> + switch (rule_type) { >>>> + case LANDLOCK_RULE_PATH_BENEATH: >>>> + node = ruleset->root_inode.rb_node; >>>> + break; >>>> + default: >>>> + return ERR_PTR(-EINVAL); >>> >>> This is a bug. There is no check for such value. You need to check >>> and update all call sites to catch such errors. Same for all new use >>> of ERR_PTR(). >> >> Sorry, I did not get your point. >> Do you mean I should check the correctness of rule_type in above >> function which calls landlock_find_rule() ??? Why can't I add such >> check here? > > landlock_find_rule() only returns NULL or a valid pointer, not an error. What about incorrect rule_type?? Return NULL? Or final rule_checl must be in upper function? > > [...] > .
On 22/03/2022 13:33, Konstantin Meskhidze wrote: > > > 3/18/2022 9:33 PM, Mickaël Salaün пишет: >> >> On 17/03/2022 15:29, Konstantin Meskhidze wrote: >>> >>> >>> 3/16/2022 11:27 AM, Mickaël Salaün пишет: >>>> >>>> On 09/03/2022 14:44, Konstantin Meskhidze wrote: >>>>> A new object union added to support a socket port >>>>> rule type. To support it landlock_insert_rule() and >>>>> landlock_find_rule() were refactored. 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> >>>>> --- [...] >>>>> @@ -156,26 +166,38 @@ 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, >>>>> + const uintptr_t object_data, >> >> Can you move rule_type here for this function and similar ones? It >> makes sense to group object-related arguments. > > Just to group them together, not putting rule_type in the end? Yes [...] >>>>> @@ -465,20 +501,28 @@ struct landlock_ruleset *landlock_merge_ruleset( >>>>> */ >>>>> 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) >>>> >>>> object_data can be 0. You need to add a test with such value. >>>> >>>> We need to be sure that this change cannot affect the current FS code. >>> >>> I got it. I will refactor it. >> >> Well, 0 means a port 0, which might not be correct, but this check >> should not be performed by landlock_merge_ruleset(). >> > Do you mean landlock_find_rule()?? Cause this check is not > performed in landlock_merge_ruleset(). Yes, I was thinking about landlock_find_rule(). If you run your tests with the patch I proposed, you'll see that one of these tests will fail (when port equal 0). When creating a new network rule, add_rule_net_service() should check if the port value is valid. However, the above `if (!object_data)` is not correct anymore. The remaining question is: should we need to accept 0 as a valid TCP port? Can it be used? How does the kernel handle it? > >> >>>> >>>> >>>>> return NULL; >>>>> - node = ruleset->root.rb_node; >>>>> + >>>>> + switch (rule_type) { >>>>> + case LANDLOCK_RULE_PATH_BENEATH: >>>>> + node = ruleset->root_inode.rb_node; >>>>> + break; >>>>> + default: >>>>> + return ERR_PTR(-EINVAL); >>>> >>>> This is a bug. There is no check for such value. You need to check >>>> and update all call sites to catch such errors. Same for all new use >>>> of ERR_PTR(). >>> >>> Sorry, I did not get your point. >>> Do you mean I should check the correctness of rule_type in above >>> function which calls landlock_find_rule() ??? Why can't I add such >>> check here? >> >> landlock_find_rule() only returns NULL or a valid pointer, not an error. > > What about incorrect rule_type?? Return NULL? Or final rule_checl > must be in upper function? This case should never happen anyway. You should return NULL and call WARN_ON_ONCE(1) just before. The same kind of WARN_ON_ONCE(1) call should be part of all switch/cases of rule_type (except the two valid values of course).
3/22/2022 4:24 PM, Mickaël Salaün пишет: > > On 22/03/2022 13:33, Konstantin Meskhidze wrote: >> >> >> 3/18/2022 9:33 PM, Mickaël Salaün пишет: >>> >>> On 17/03/2022 15:29, Konstantin Meskhidze wrote: >>>> >>>> >>>> 3/16/2022 11:27 AM, Mickaël Salaün пишет: >>>>> >>>>> On 09/03/2022 14:44, Konstantin Meskhidze wrote: >>>>>> A new object union added to support a socket port >>>>>> rule type. To support it landlock_insert_rule() and >>>>>> landlock_find_rule() were refactored. 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> >>>>>> --- > > [...] > >>>>>> @@ -156,26 +166,38 @@ 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, >>>>>> + const uintptr_t object_data, >>> >>> Can you move rule_type here for this function and similar ones? It >>> makes sense to group object-related arguments. >> >> Just to group them together, not putting rule_type in the end? > > Yes Ok. Got it. > > [...] > >>>>>> @@ -465,20 +501,28 @@ struct landlock_ruleset >>>>>> *landlock_merge_ruleset( >>>>>> */ >>>>>> 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) >>>>> >>>>> object_data can be 0. You need to add a test with such value. >>>>> >>>>> We need to be sure that this change cannot affect the current FS code. >>>> >>>> I got it. I will refactor it. >>> >>> Well, 0 means a port 0, which might not be correct, but this check >>> should not be performed by landlock_merge_ruleset(). >>> >> Do you mean landlock_find_rule()?? Cause this check is not >> performed in landlock_merge_ruleset(). > > Yes, I was thinking about landlock_find_rule(). If you run your tests > with the patch I proposed, you'll see that one of these tests will fail > (when port equal 0). When creating a new network rule, > add_rule_net_service() should check if the port value is valid. However, > the above `if (!object_data)` is not correct anymore. > > The remaining question is: should we need to accept 0 as a valid TCP > port? Can it be used? How does the kernel handle it? I agree that must be a check for port 0 in add_rule_net_service(), cause unlike most port numbers, port 0 is a reserved port in TCP/IP networking, meaning that it should not be used in TCP or UDP messages. Also network traffic sent across the internet to hosts listening on port 0 might be generated from network attackers or accidentally by applications programmed incorrectly. Source: https://www.lifewire.com/port-0-in-tcp-and-udp-818145 > >> >>> >>>>> >>>>> >>>>>> return NULL; >>>>>> - node = ruleset->root.rb_node; >>>>>> + >>>>>> + switch (rule_type) { >>>>>> + case LANDLOCK_RULE_PATH_BENEATH: >>>>>> + node = ruleset->root_inode.rb_node; >>>>>> + break; >>>>>> + default: >>>>>> + return ERR_PTR(-EINVAL); >>>>> >>>>> This is a bug. There is no check for such value. You need to check >>>>> and update all call sites to catch such errors. Same for all new >>>>> use of ERR_PTR(). >>>> >>>> Sorry, I did not get your point. >>>> Do you mean I should check the correctness of rule_type in above >>>> function which calls landlock_find_rule() ??? Why can't I add such >>>> check here? >>> >>> landlock_find_rule() only returns NULL or a valid pointer, not an error. >> >> What about incorrect rule_type?? Return NULL? Or final rule_checl >> must be in upper function? > > This case should never happen anyway. You should return NULL and call > WARN_ON_ONCE(1) just before. The same kind of WARN_ON_ONCE(1) call > should be part of all switch/cases of rule_type (except the two valid > values of course). Ok. I got it. Thanks. > .
On 23/03/2022 09:41, Konstantin Meskhidze wrote: > > > 3/22/2022 4:24 PM, Mickaël Salaün пишет: >> [...] >> The remaining question is: should we need to accept 0 as a valid TCP >> port? Can it be used? How does the kernel handle it? > > I agree that must be a check for port 0 in add_rule_net_service(), > cause unlike most port numbers, port 0 is a reserved port in TCP/IP > networking, meaning that it should not be used in TCP or UDP messages. > Also network traffic sent across the internet to hosts listening on port > 0 might be generated from network attackers or accidentally by > applications programmed incorrectly. > Source: https://www.lifewire.com/port-0-in-tcp-and-udp-818145 OK, so denying this port by default without a way to allow it should not be an issue. I guess an -EINVAL error would make sense when trying to allow this port. This should be documented in a comment (with a link to the RFC/section) and a dedicated test should check that behavior. What is the behavior of firewalls (e.g. Netfiler) when trying to filter port 0? This doesn't seem to be settle though: https://www.austingroupbugs.net/view.php?id=1068 Interesting article: https://z3r0trust.medium.com/socket-programming-the-bizarre-tcp-ip-port-0-saga-fcfbc0e0a276
4/12/2022 2:07 PM, Mickaël Salaün пишет: > > On 23/03/2022 09:41, Konstantin Meskhidze wrote: >> >> >> 3/22/2022 4:24 PM, Mickaël Salaün пишет: >>> > > [...] >>> The remaining question is: should we need to accept 0 as a valid TCP >>> port? Can it be used? How does the kernel handle it? >> >> I agree that must be a check for port 0 in add_rule_net_service(), >> cause unlike most port numbers, port 0 is a reserved port in TCP/IP >> networking, meaning that it should not be used in TCP or UDP messages. >> Also network traffic sent across the internet to hosts listening on >> port 0 might be generated from network attackers or accidentally by >> applications programmed incorrectly. >> Source: https://www.lifewire.com/port-0-in-tcp-and-udp-818145 > > OK, so denying this port by default without a way to allow it should not > be an issue. I guess an -EINVAL error would make sense when trying to > allow this port. This should be documented in a comment (with a link to > the RFC/section) and a dedicated test should check that behavior. > > What is the behavior of firewalls (e.g. Netfiler) when trying to filter > port 0? To be honest I don't know. I'm trying to check it. > > This doesn't seem to be settle though: > https://www.austingroupbugs.net/view.php?id=1068 > > Interesting article: > https://z3r0trust.medium.com/socket-programming-the-bizarre-tcp-ip-port-0-saga-fcfbc0e0a276 Thanks. I will check. > > .
diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 97f5c455f5a7..1497948d754f 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -168,7 +168,7 @@ 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() @@ -195,7 +195,8 @@ static inline u64 unmask_layers( inode = d_backing_inode(path->dentry); rcu_read_lock(); rule = landlock_find_rule(domain, - rcu_dereference(landlock_inode(inode)->object)); + (uintptr_t)rcu_dereference(landlock_inode(inode)->object), + LANDLOCK_RULE_PATH_BENEATH); rcu_read_unlock(); if (!rule) return layer_mask; diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c index a6212b752549..971685c48641 100644 --- a/security/landlock/ruleset.c +++ b/security/landlock/ruleset.c @@ -34,7 +34,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 @@ -81,10 +81,12 @@ static void build_check_rule(void) } static struct landlock_rule *create_rule( - struct landlock_object *const object, + 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) + const struct landlock_layer *const new_layer, + const u16 rule_type) { struct landlock_rule *new_rule; u32 new_num_layers; @@ -103,8 +105,16 @@ static struct landlock_rule *create_rule( if (!new_rule) return ERR_PTR(-ENOMEM); RB_CLEAR_NODE(&new_rule->node); - landlock_get_object(object); - new_rule->object = object; + + switch (rule_type) { + case LANDLOCK_RULE_PATH_BENEATH: + landlock_get_object(object_ptr); + new_rule->object.ptr = object_ptr; + break; + default: + return ERR_PTR(-EINVAL); + } + new_rule->num_layers = new_num_layers; /* Copies the original layer stack. */ memcpy(new_rule->layers, layers, @@ -120,7 +130,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); } @@ -156,26 +166,38 @@ 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, + const uintptr_t object_data, const struct landlock_layer (*const layers)[], - size_t num_layers) + size_t num_layers, u16 rule_type) { struct rb_node **walker_node; struct rb_node *parent_node = NULL; struct landlock_rule *new_rule; + uintptr_t object; + struct rb_root *root; might_sleep(); lockdep_assert_held(&ruleset->lock); - if (WARN_ON_ONCE(!object || !layers)) - return -ENOENT; - walker_node = &(ruleset->root.rb_node); + /* Choose rb_tree structure depending on a rule type */ + switch (rule_type) { + case LANDLOCK_RULE_PATH_BENEATH: + if (WARN_ON_ONCE(!object_ptr || !layers)) + return -ENOENT; + object = (uintptr_t)object_ptr; + root = &ruleset->root_inode; + break; + default: + 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) { parent_node = *walker_node; - if (this->object < object) + if (this->object.data < object) walker_node = &((*walker_node)->rb_right); else walker_node = &((*walker_node)->rb_left); @@ -207,11 +229,15 @@ 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], rule_type); + 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; } @@ -220,11 +246,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, rule_type); + 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; } @@ -242,7 +272,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 u32 access) + struct landlock_object *const object_ptr, + const uintptr_t object_data, + const u32 access, const u16 rule_type) { struct landlock_layer layers[] = {{ .access = access, @@ -251,7 +283,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, &layers, + ARRAY_SIZE(layers), rule_type); } static inline void get_hierarchy(struct landlock_hierarchy *const hierarchy) @@ -297,7 +330,7 @@ static int merge_ruleset(struct landlock_ruleset *const dst, /* Merges the @src tree. */ rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, - &src->root, node) { + &src->root_inode, node) { struct landlock_layer layers[] = {{ .level = dst->num_layers, }}; @@ -311,8 +344,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, - ARRAY_SIZE(layers)); + err = insert_rule(dst, walker_rule->object.ptr, 0, &layers, + ARRAY_SIZE(layers), LANDLOCK_RULE_PATH_BENEATH); if (err) goto out_unlock; } @@ -323,6 +356,8 @@ static int merge_ruleset(struct landlock_ruleset *const dst, return err; } + + static int inherit_ruleset(struct landlock_ruleset *const parent, struct landlock_ruleset *const child) { @@ -339,9 +374,10 @@ 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, - &walker_rule->layers, walker_rule->num_layers); + &parent->root_inode, node) { + err = insert_rule(child, walker_rule->object.ptr, 0, + &walker_rule->layers, walker_rule->num_layers, + LANDLOCK_RULE_PATH_BENEATH); if (err) goto out_unlock; } @@ -372,7 +408,7 @@ 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, + rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root_inode, node) free_rule(freeme); put_hierarchy(ruleset->hierarchy); @@ -465,20 +501,28 @@ struct landlock_ruleset *landlock_merge_ruleset( */ 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: + return ERR_PTR(-EINVAL); + } + 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 bc87e5f787f7..088b8d95f653 100644 --- a/security/landlock/ruleset.h +++ b/security/landlock/ruleset.h @@ -50,15 +50,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 socket port object. 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 (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 @@ -95,7 +97,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. @@ -157,7 +159,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 u32 access); + struct landlock_object *const object_ptr, + const uintptr_t object_data, + const u32 access, const u16 rule_type); struct landlock_ruleset *landlock_merge_ruleset( struct landlock_ruleset *const parent, @@ -165,7 +169,7 @@ struct landlock_ruleset *landlock_merge_ruleset( 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) {
A new object union added to support a socket port rule type. To support it landlock_insert_rule() and landlock_find_rule() were refactored. 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 v3: * Split commit. * Refactoring landlock_insert_rule and landlock_find_rule functions. * Rename new_ruleset->root_inode. --- security/landlock/fs.c | 5 +- security/landlock/ruleset.c | 108 +++++++++++++++++++++++++----------- security/landlock/ruleset.h | 26 +++++---- 3 files changed, 94 insertions(+), 45 deletions(-) -- 2.25.1