Message-ID: <8683502f-113d-fafd-be85-30cf829fb018@digikod.net>
Date:   Thu, 19 May 2022 16:37:17 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     Konstantin Meskhidze <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 v5 05/15] landlock: landlock_add_rule syscall refactoring



On 19/05/2022 11:23, Konstantin Meskhidze wrote:
> 
> 
> 5/17/2022 11:04 AM, Mickaël Salaün пишет:
>> You can rename the subject to "landlock: Refactor landlock_add_rule()"
>>
>>
>> On 16/05/2022 17:20, Konstantin Meskhidze wrote:
>>> Landlock_add_rule syscall was refactored to support new
>>> rule types in future Landlock versions. Add_rule_path_beneath()
>>
>> nit: add_rule_path_beneath(), not Add_rule_path_beneath()
>>
>    Ok. Thanks. Will be renamed.
> 
>>> helper was added to support current filesystem rules. It is called
>>> by the switch case.
>>
>> You can rephrase (all commit messages) in the present form:
>>
>> Refactor the landlock_add_rule() syscall with add_rule_path_beneath() 
>> to support new…
>>
>> Refactor the landlock_add_rule() syscall to easily support for a new 
>> rule type in a following commit. The new add_rule_path_beneath() 
>> helper supports current filesystem rules.
>>
>    Ok. I will fix it.
>>
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
>>> ---
>>>
>>> Changes since v3:
>>> * Split commit.
>>> * Refactoring landlock_add_rule syscall.
>>>
>>> Changes since v4:
>>> * Refactoring add_rule_path_beneath() and landlock_add_rule() functions
>>> to optimize code usage.
>>> * Refactoring base_test.c seltest: adds LANDLOCK_RULE_PATH_BENEATH
>>> rule type in landlock_add_rule() call.
>>>
>>> ---
>>>   security/landlock/syscalls.c                 | 105 ++++++++++---------
>>>   tools/testing/selftests/landlock/base_test.c |   4 +-
>>>   2 files changed, 59 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>>> index 1db799d1a50b..412ced6c512f 100644
>>> --- a/security/landlock/syscalls.c
>>> +++ b/security/landlock/syscalls.c
>>> @@ -274,67 +274,23 @@ static int get_path_from_fd(const s32 fd, 
>>> struct path *const path)
>>>       return err;
>>>   }
>>>
>>> -/**
>>> - * sys_landlock_add_rule - Add a new rule to a ruleset
>>> - *
>>> - * @ruleset_fd: File descriptor tied to the ruleset that should be 
>>> extended
>>> - *        with the new rule.
>>> - * @rule_type: Identify the structure type pointed to by @rule_attr 
>>> (only
>>> - *             LANDLOCK_RULE_PATH_BENEATH for now).
>>> - * @rule_attr: Pointer to a rule (only of type &struct
>>> - *             landlock_path_beneath_attr for now).
>>> - * @flags: Must be 0.
>>> - *
>>> - * This system call enables to define a new rule and add it to an 
>>> existing
>>> - * ruleset.
>>> - *
>>> - * Possible returned errors are:
>>> - *
>>> - * - EOPNOTSUPP: Landlock is supported by the kernel but disabled at 
>>> boot time;
>>> - * - EINVAL: @flags is not 0, or inconsistent access in the rule (i.e.
>>> - *   &landlock_path_beneath_attr.allowed_access is not a subset of the
>>> - *   ruleset handled accesses);
>>> - * - ENOMSG: Empty accesses (e.g. 
>>> &landlock_path_beneath_attr.allowed_access);
>>> - * - EBADF: @ruleset_fd is not a file descriptor for the current 
>>> thread, or a
>>> - *   member of @rule_attr is not a file descriptor as expected;
>>> - * - EBADFD: @ruleset_fd is not a ruleset file descriptor, or a 
>>> member of
>>> - *   @rule_attr is not the expected file descriptor type;
>>> - * - EPERM: @ruleset_fd has no write access to the underlying ruleset;
>>> - * - EFAULT: @rule_attr inconsistency.
>>> - */
>>> -SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>>> -        const enum landlock_rule_type, rule_type,
>>> -        const void __user *const, rule_attr, const __u32, flags)
>>> +static int add_rule_path_beneath(const int ruleset_fd, const void 
>>> *const rule_attr)
>>>   {
>>>       struct landlock_path_beneath_attr path_beneath_attr;
>>>       struct path path;
>>>       struct landlock_ruleset *ruleset;
>>>       int res, err;
>>>
>>> -    if (!landlock_initialized)
>>> -        return -EOPNOTSUPP;
>>> -
>>> -    /* No flag for now. */
>>> -    if (flags)
>>> -        return -EINVAL;
>>> -
>>>       /* Gets and checks the ruleset. */
>>
>> Like I already said, this needs to stay in landlock_add_rule(). I 
>> think there is some inconsistencies with other patches that rechange 
>> this part. Please review your patches and make clean patches that 
>> don't partially revert the previous ones.
>>
>    Do you mean to leave this code as it its till adding network part
> in commit landlock: TCP network hooks implementation?
>   In this case this patch can be dropped.

The syscall argument check ordering needs to stay in the same order as 
you can see in the add_rule_checks_ordering test. Other than that, this 
commit looks good, it just splits the syscall in two functions, which is 
useful.


>>
>>>       ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
>>>       if (IS_ERR(ruleset))
>>>           return PTR_ERR(ruleset);
>>>
>>> -    if (rule_type != LANDLOCK_RULE_PATH_BENEATH) {
>>> -        err = -EINVAL;
>>> -        goto out_put_ruleset;
>>> -    }
>>> -
>>>       /* Copies raw user space buffer, only one type for now. */
>>>       res = copy_from_user(&path_beneath_attr, rule_attr,
>>> -                 sizeof(path_beneath_attr));
>>> -    if (res) {
>>> -        err = -EFAULT;
>>> -        goto out_put_ruleset;
>>> -    }
>>> +                sizeof(path_beneath_attr));
>>> +    if (res)
>>> +        return -EFAULT;
>>>
>>>       /*
>>>        * Informs about useless rule: empty allowed_access (i.e. deny 
>>> rules)
>>> @@ -370,6 +326,59 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, 
>>> ruleset_fd,
>>>       return err;
>>>   }
>>>
>>> +/**
>>> + * sys_landlock_add_rule - Add a new rule to a ruleset
>>> + *
>>> + * @ruleset_fd: File descriptor tied to the ruleset that should be 
>>> extended
>>> + *        with the new rule.
>>> + * @rule_type: Identify the structure type pointed to by @rule_attr 
>>> (only
>>> + *             LANDLOCK_RULE_PATH_BENEATH for now).
>>> + * @rule_attr: Pointer to a rule (only of type &struct
>>> + *             landlock_path_beneath_attr for now).
>>> + * @flags: Must be 0.
>>> + *
>>> + * This system call enables to define a new rule and add it to an 
>>> existing
>>> + * ruleset.
>>> + *
>>> + * Possible returned errors are:
>>> + *
>>> + * - EOPNOTSUPP: Landlock is supported by the kernel but disabled at 
>>> boot time;
>>> + * - EINVAL: @flags is not 0, or inconsistent access in the rule (i.e.
>>> + *   &landlock_path_beneath_attr.allowed_access is not a subset of 
>>> the rule's
>>> + *   accesses);
>>> + * - ENOMSG: Empty accesses (e.g. 
>>> &landlock_path_beneath_attr.allowed_access);
>>> + * - EBADF: @ruleset_fd is not a file descriptor for the current 
>>> thread, or a
>>> + *   member of @rule_attr is not a file descriptor as expected;
>>> + * - EBADFD: @ruleset_fd is not a ruleset file descriptor, or a 
>>> member of
>>> + *   @rule_attr is not the expected file descriptor type (e.g. file 
>>> open
>>> + *   without O_PATH);
>>> + * - EPERM: @ruleset_fd has no write access to the underlying ruleset;
>>> + * - EFAULT: @rule_attr inconsistency.
>>> + */
>>> +SYSCALL_DEFINE4(landlock_add_rule,
>>> +        const int, ruleset_fd, const enum landlock_rule_type, 
>>> rule_type,
>>> +        const void __user *const, rule_attr, const __u32, flags)
>>> +{
>>> +    int err;
>>> +
>>> +    if (!landlock_initialized)
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    /* No flag for now. */
>>> +    if (flags)
>>> +        return -EINVAL;
>>> +
>>> +    switch (rule_type) {
>>> +    case LANDLOCK_RULE_PATH_BENEATH:
>>> +        err = add_rule_path_beneath(ruleset_fd, rule_attr);
>>> +        break;
>>> +    default:
>>> +        err = -EINVAL;
>>> +        break;
>>> +    }
>>> +    return err;
>>> +}
>>> +
>>>   /* Enforcement */
>>>
>>>   /**
>>> diff --git a/tools/testing/selftests/landlock/base_test.c 
>>> b/tools/testing/selftests/landlock/base_test.c
>>> index da9290817866..0c4c3a538d54 100644
>>> --- a/tools/testing/selftests/landlock/base_test.c
>>> +++ b/tools/testing/selftests/landlock/base_test.c
>>> @@ -156,11 +156,11 @@ TEST(add_rule_checks_ordering)
>>>       ASSERT_LE(0, ruleset_fd);
>>>
>>>       /* Checks invalid flags. */
>>> -    ASSERT_EQ(-1, landlock_add_rule(-1, 0, NULL, 1));
>>> +    ASSERT_EQ(-1, landlock_add_rule(-1, LANDLOCK_RULE_PATH_BENEATH, 
>>> NULL, 1));
>>
>> This must not be changed! I specifically added these tests to make 
>> sure no one change the argument ordering checks…
> 
>    I updated this code cause I got error in base_test.
>    Ok. But in future commints I will order funtions calls in
>    landlock_add_rule() so that base_test runs smoothly (ordering checks).

Right, these tests are correct and they can help you.


> 
>>
>>
>>>       ASSERT_EQ(EINVAL, errno);
>>>
>>>       /* Checks invalid ruleset FD. */
>>> -    ASSERT_EQ(-1, landlock_add_rule(-1, 0, NULL, 0));
>>> +    ASSERT_EQ(-1, landlock_add_rule(-1, LANDLOCK_RULE_PATH_BENEATH, 
>>> NULL, 0));
>>>       ASSERT_EQ(EBADF, errno);
>>>
>>>       /* Checks invalid rule type. */
>>> -- 
>>> 2.25.1
>>>
>> .

Powered by blists - more mailing lists