Re: [PATCH v6 02/17] landlock: refactors landlock_find/insert_rule

From: "Konstantin Meskhidze (A)" <konstantin.meskhidze@huawei.com>
To: "Mickaël Salaün" <mic@digikod.net>
Cc: <willemdebruijn.kernel@gmail.com>,
	<linux-security-module@vger.kernel.org>, <netdev@vger.kernel.org>,
	<netfilter-devel@vger.kernel.org>, <yusongping@huawei.com>,
	<anton.sirazetdinov@huawei.com>
Subject: Re: [PATCH v6 02/17] landlock: refactors landlock_find/insert_rule
Date: Fri, 8 Jul 2022 17:14:35 +0300	[thread overview]
Message-ID: <92bb77ab-ec5a-07cc-ef2e-bf6c15e94478@huawei.com> (raw)
In-Reply-To: <72375435-94d4-e3aa-c27b-b44382dde6ad@digikod.net>


7/8/2022 4:56 PM, Mickaël Salaün пишет:
> 
> On 08/07/2022 14:53, 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@huawei.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?
>> 
>> It was your suggestion to use it like this:
>> " ....You can also add a WARN_ON_ONCE(object_ptr && object_data)."
>> 
>> Please check it here:
>> https://lore.kernel.org/linux-security-module/bc44f11f-0eaa-a5f6-c5dc-1d36570f1be1@digikod.net/ 
> 
> Yes, but the error is in the "else", you should write:
> if (WARN_ON_ONCE(object_ptr && object_data))
> 	return ERR_PTR(-EINVAL);
> 
> …and this should be before the `if (object_ptr) {` line (to avoid
> erronous landlock_get_object() call), just after the `if (!new_rule)` check.

  By the way, in the next commits I have fixed this logic error.
Anyway I will refactor this one also. Thanks.
> .