netdev - Re: [PATCH v6 02/17] landlock: refactors landlock_find/insert

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b08fe5cc-3be0-390b-3575-4f27f795f609@digikod.net>
Date:   Fri, 8 Jul 2022 15:59:24 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>
Cc:     willemdebruijn.kernel@...il.com,
        linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org, yusongping@...wei.com,
        anton.sirazetdinov@...wei.com
Subject: Re: [PATCH v6 02/17] landlock: refactors landlock_find/insert_rule


On 08/07/2022 15:10, Konstantin Meskhidze (A) wrote:
> 
> 
> 7/7/2022 7:44 PM, Mickaël Salaün пишет:
>>
>> On 21/06/2022 10:22, Konstantin Meskhidze wrote:
>>> Adds a new object union to support a socket port
>>> rule type. Refactors landlock_insert_rule() and
>>> landlock_find_rule() to support coming network
>>> modifications. 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@...wei.com>
>>> ---
>>>
>>> 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      |   7 ++-
>>>   security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
>>>   security/landlock/ruleset.h |  27 +++++-----
>>>   3 files changed, 96 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>> index e6da08ed99d1..46aedc2a05a8 100644
>>> --- a/security/landlock/fs.c
>>> +++ b/security/landlock/fs.c
>>> @@ -173,7 +173,8 @@ 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()
>>> @@ -204,7 +205,9 @@ 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));
>>> +        domain,
>>> +        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
>>> +        LANDLOCK_RULE_PATH_BENEATH);
>>>       rcu_read_unlock();
>>>       return rule;
>>>   }
>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>> index a3fd58d01f09..5f13f8a12aee 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
>>> @@ -69,7 +69,8 @@ static void build_check_rule(void)
>>>   }
>>>
>>>   static struct landlock_rule *
>>> -create_rule(struct landlock_object *const object,
>>> +create_rule(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)
>>>   {
>>> @@ -90,8 +91,15 @@ 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 (object_ptr) {
>>> +        landlock_get_object(object_ptr);
>>> +        new_rule->object.ptr = object_ptr;
>>> +    } else if (object_ptr && object_data) {
>>
>> Something is wrong with this second check: else + object_ptr?
> 
>   Sorry. Do you mean logical error here? I got your point.
>   You are right!
> 
>   I think it must be refactored like this:
> 
>      if (object_ptr && !object_data) {
>          landlock_get_object(object_ptr);
>          new_rule->object.ptr = object_ptr;
>      } else if (object_ptr && object_data) {
>          ...
>      }

There is indeed a logical error but this doesn't fix everything. Please 
include my previous suggestion instead.


> Plus, I will add a test for this case.

That would be great but I don't think this code is reachable from user 
space. I think that would require kunit but I may be missing something. 
How would you test this?