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

[Mickaël Salaün]

On 11/07/2022 16:05, Konstantin Meskhidze (A) wrote:
> 
> 
> 7/8/2022 5:35 PM, Mickaël Salaün пишет:
>>
>> 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 at 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?
>>>>>
>>>>>    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.
> 
>    I'm thinking about ones that we can't reach from the userspace.

Right, some lines cannot be reached by user space because they are logically
not possible but safety checks in case of unexpected kernel code modification
(which, unfortunately, cannot be done at build time).


>    I analyzed test coverage logs finding lines that are untouched by the
> userspace tests.
>    Let's discus this list:
> 
> 	1. create_rule():  - insert both  object_ptr and object_data.

This check is gone with my last patch.

> 
> 	2. insert_rule():  - insert both  object_ptr and object_data.

This check is gone with my last patch.


> 			   - insert NULL (*const layers).
> 			   - insert layers[0].level != 0.
> 			   - insert num_layers != 1.
> 			   - insert default rule_type.
> 	
> 	3. tree_merge():   - insert default rule_type.
> 			   - insert walker_rule->num_layers != 1.
> 			   - insert walker_rule->layers[0].level != 0.
> 	
> 	4. tree_copy():    - insert default rule_type.
> 	
> 	5. merge_ruleset:  - insert !dst || !dst->hierarchy.
> 			   - insert src->num_layers != 1 ||
>                                       dst->num_layers < 1.
> 
> 	6. inherit_ruleset(): - insert child->num_layers <=
> 				   parent->num_layers.
>    			      - insert parent->hierarchy = NULL.
> 
> 	7. landlock_merge_ruleset(): - insert ruleset = NULL.
> 				     - insert parent = ruleset
> 
> 	8. landlock_find_rule(): - insert default rule_type.
> 
>    Please your opinion?

All these checks are enclosed with WARN_ON_ONCE(). I'm not sure it will help
to add kunit tests for them. It could be interesting to add kunit test to
check the behavior of standalone helpers though, e.g. managing rulesets or
trees.