From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4C5FDC433EF for ; Tue, 22 Mar 2022 12:33:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233049AbiCVMfI (ORCPT ); Tue, 22 Mar 2022 08:35:08 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44890 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233562AbiCVMfH (ORCPT ); Tue, 22 Mar 2022 08:35:07 -0400 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 34D7925E8E; Tue, 22 Mar 2022 05:33:38 -0700 (PDT) Received: from fraeml704-chm.china.huawei.com (unknown [172.18.147.200]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4KN9pg3zwwz688fv; Tue, 22 Mar 2022 20:31:55 +0800 (CST) Received: from [10.122.132.241] (10.122.132.241) by fraeml704-chm.china.huawei.com (10.206.15.53) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2375.24; Tue, 22 Mar 2022 13:33:35 +0100 Message-ID: Date: Tue, 22 Mar 2022 15:33:17 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.4.1 Subject: Re: [RFC PATCH v4 03/15] landlock: landlock_find/insert_rule refactoring Content-Language: ru To: =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= , CC: , , , , , References: <20220309134459.6448-1-konstantin.meskhidze@huawei.com> <20220309134459.6448-4-konstantin.meskhidze@huawei.com> <6535183b-5fad-e3a9-1350-d22122205be6@huawei.com> <92d498f0-c598-7413-6b7c-d19c5aec6cab@digikod.net> From: Konstantin Meskhidze In-Reply-To: <92d498f0-c598-7413-6b7c-d19c5aec6cab@digikod.net> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.122.132.241] X-ClientProxiedBy: lhreml753-chm.china.huawei.com (10.201.108.203) To fraeml704-chm.china.huawei.com (10.206.15.53) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org 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 >>>> --- >>>> >>>> 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? > > [...] > .