| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <88cd33f2-8c6c-bfff-7271-7508b1ebac17@huawei.com>
Date: Mon, 11 Jul 2022 11:16:37 +0300
From: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>
To: Mickaël Salaün <mic@...ikod.net>
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
7/8/2022 7:57 PM, Mickaël Salaün пишет:
>
> On 08/07/2022 16:20, Konstantin Meskhidze (A) wrote:
>>
>>
>> 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@...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?
>>>>
>>>> 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.
Ok. I got it.
>>
>> Maybe we could delete this check here cause we have it in the upper
>> insert_rule() function??
>>
>> ...
>> if (WARN_ON_ONCE(!layers))
>> return -ENOENT;
>> ------> if (WARN_ON_ONCE(object_ptr && object_data))
>> return -EINVAL;
>> /* Chooses rb_tree structure depending on a rule type. */
>> switch (rule_type) {
>> case LANDLOCK_RULE_PATH_BENEATH:
>> if (WARN_ON_ONCE(!object_ptr))
>> return -ENOENT;
>> object_data = (uintptr_t)object_ptr;
>> root = &ruleset->root_inode;
>> break;
>> default:
>> WARN_ON_ONCE(1);
>> return -EINVAL;
>> }
>> ...
>>
>> This is double check here. What do you think?
>
> This check is indeed done twice, and for now create_rule() is only
> called from insert_rule(), but I prefer to keep it in both location to
> not get bitten in the future were it could be called from other
> locations. The compiler may be smart enough to remove the redundant
> checks though.
>
> I'll send a patch to improve this part.
Ok. thanks!!!
> .
Powered by blists - more mailing lists