| 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: <582f8ace-1f95-16a6-fa9e-4014ddd8b7f2@digikod.net>
Date: Fri, 8 Jul 2022 16:35:12 +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 16:14, Konstantin Meskhidze (A) wrote:
>
>
> 7/8/2022 4:59 PM, Mickaël Salaün пишет:
>>
>> 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.
>>
> By the way, in the next commits I have fixed this logic error.
> Anyway I will refactor this one also. Thanks.
>>
>>> 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?
>
> You are correct. I checked it. It's impossible to reach this line from
> userpace (insert both object_ptr and object_data). But create_rule()
> must be used carefuly by other developers (if any in future). Do you
> think if its possible to have some internal kernel tests that could
> handle this issue?
We can use kunit tests for such kernel functions, but in this case I'm
not sure what could be tested. I started working on bringing kunit tests
to Landlock but it's not ready yet. Please list all non-userspace tests
you can think about.
Powered by blists - more mailing lists