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

["Konstantin Meskhidze (A)" <konstantin.meskhidze@huawei.com>]

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@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.
  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.

	2. insert_rule():  - insert both  object_ptr and object_data.
			   - 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?
> .