Re: [PATCH v5 05/15] landlock: landlock_add_rule syscall refactoring

From: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
To: "Mickaël Salaün" <mic@digikod.net>
Cc: <willemdebruijn.kernel@gmail.com>,
	<linux-security-module@vger.kernel.org>, <netdev@vger.kernel.org>,
	<netfilter-devel@vger.kernel.org>, <yusongping@huawei.com>,
	<anton.sirazetdinov@huawei.com>
Subject: Re: [PATCH v5 05/15] landlock: landlock_add_rule syscall refactoring
Date: Tue, 24 May 2022 11:35:30 +0300	[thread overview]
Message-ID: <9f3035e4-cf9b-74d6-e77e-a2676c76d9a8@huawei.com> (raw)
In-Reply-To: <8683502f-113d-fafd-be85-30cf829fb018@digikod.net>


5/19/2022 5:37 PM, Mickaël Salaün пишет:
> 
> 
> 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@huawei.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.
> 
  Thank 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
>>>>
>>> .
> .