Message-ID: <3a59d29e-8c3e-cb34-ae3d-9961005780fd@huawei.com>
Date:   Wed, 9 Feb 2022 06:06:30 +0300
From:   Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
To:     Mickaël Salaün <mic@...ikod.net>
CC:     <linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
        <netfilter@...r.kernel.org>, <yusongping@...wei.com>,
        <artem.kuzin@...wei.com>
Subject: Re: [RFC PATCH 1/2] landlock: TCP network hooks implementation



2/8/2022 3:09 PM, Mickaël Salaün пишет:
> 
> On 08/02/2022 08:55, Konstantin Meskhidze wrote:
>>
>>
>> 2/7/2022 5:17 PM, Mickaël Salaün пишет:
>>>
>>> On 07/02/2022 14:09, Konstantin Meskhidze wrote:
>>>>
>>>>
>>>> 2/1/2022 3:13 PM, Mickaël Salaün пишет:
>>>>>
>>>>> On 24/01/2022 09:02, Konstantin Meskhidze wrote:
>>>>>> Support of socket_bind() and socket_connect() hooks.
>>>>>> Current prototype can restrict binding and connecting of TCP
>>>>>> types of sockets. Its just basic idea how Landlock could support
>>>>>> network confinement.
>>>>>>
>>>>>> Changes:
>>>>>> 1. Access masks array refactored into 1D one and changed
>>>>>> to 32 bits. Filesystem masks occupy 16 lower bits and network
>>>>>> masks reside in 16 upper bits.
>>>>>> 2. Refactor API functions in ruleset.c:
>>>>>>      1. Add void *object argument.
>>>>>>      2. Add u16 rule_type argument.
>>>>>> 3. Use two rb_trees in ruleset structure:
>>>>>>      1. root_inode - for filesystem objects
>>>>>>      2. root_net_port - for network port objects
>>>>>
>>>>> It's good to add a changelog, but they must not be in commit 
>>>>> messages that get copied by git am. Please use "---" to separate 
>>>>> this additionnal info (but not the Signed-off-by). Please also 
>>>>> include a version in the email subjects (this one should have been 
>>>>> "[RFC PATCH v3 1/2] landlock: …"), e.g. using git format-patch 
>>>>> --reroll-count=X .
>>>>>
>>>>> Please follow these rules: 
>>>>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>>>>> You can take some inspiration from this patch series: 
>>>>> https://lore.kernel.org/lkml/20210422154123.13086-1-mic@digikod.net/
>>>>
>>>>   Ok. I will add patch vervison in next patch. So it will be "[RFC 
>>>> PATCH
>>>>   v4 ../..] landlock: ..."
>>>>   But the previous patches remain with no version, correct?
>>>
>>> Right, you can't change the subject of already sent emails. ;)
>>
>>    Ok. But I can add previous patches like:
>>     v1: 
>> https://lore.kernel.org/linux-security-module/20211210072123.386713-1-konstantin.meskhidze@huawei.com 
>>
>>     v2: 
>> https://lore.kernel.org/netdev/20211228115212.703084-1-konstantin.meskhidze@huawei.com/ 
>>
>>     v3: ....
>>
>>   right ?
> 
> Absolutely! This is a good practice (and would be better in reverse order).
> 
   Thanks!
> 
>>> [...]
>>>
>>>>>> @@ -67,10 +76,11 @@ static void build_check_rule(void)
>>>>>>   }
>>>>>>   static struct landlock_rule *create_rule(
>>>>>> -        struct landlock_object *const object,
>>>>>> +        void *const object,
>>>>>
>>>>> Instead of shoehorning two different types into one (and then 
>>>>> loosing the typing), you should rename object to object_ptr and add 
>>>>> a new object_data argument. Only one of these should be set 
>>>>> according to the rule_type. However, if there is no special action 
>>>>> performed on one of these type (e.g. landlock_get_object), only one 
>>>>> uintptr_t argument should be enough.
>>>>>
>>>>   Do you mean using 2 object arguments in create_rule():
>>>>
>>>>      1. create_rule( object_ptr = landlock_object , object_data = 0,
>>>>                                 ...,  fs_rule_type);
>>>>          2. create_rule( object_ptr = NULL , object_data = port, .... ,
>>>>                           net_rule_type);
>>>
>>> Yes, and you can add a WARN_ON_ONCE() in these function to check that 
>>> only one argument is set (but object_data could be 0 in each case). 
>>> The landlock_get_object() function should only require an object_data 
>>> though.
>>>
>>    Sorry. As you said in previous comment in landlock_get_object, only
>>    one  uintptr_t argument should be enough. But, I did not get: "The
>>    landlock_get_object() function should only require an object_data
>>    though".
>>    uintptr_t is the only argument in landlock_get_object?
> 
> I was thinking about landlock_find_rule(), not landlock_get_object():
> const struct landlock_rule *landlock_find_rule(
>          const struct landlock_ruleset *const ruleset,
>          const uintptr_t object_data)

   I got it. Thnaks.
> 
> 
>>> [...]
>>>
>>>>>> @@ -317,47 +331,91 @@ SYSCALL_DEFINE4(landlock_add_rule,
>>>>>>       if (flags)
>>>>>>           return -EINVAL;
>>>>>> -    if (rule_type != LANDLOCK_RULE_PATH_BENEATH)
>>>>>> +    if ((rule_type != LANDLOCK_RULE_PATH_BENEATH) &&
>>>>>> +        (rule_type != LANDLOCK_RULE_NET_SERVICE))
>>>>>
>>>>> Please replace with a switch/case.
>>>>
>>>>    Ok. I got it.
>>>>>
>>>>>
>>>>>>           return -EINVAL;
>>>>>> -    /* 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)
>>>>>> -        return -EFAULT;
>>>>>> -
>>>>>> -    /* Gets and checks the ruleset. */
>>>>>> -    ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
>>>>>> -    if (IS_ERR(ruleset))
>>>>>> -        return PTR_ERR(ruleset);
>>>>>> -
>>>>>> -    /*
>>>>>> -     * Informs about useless rule: empty allowed_access (i.e. 
>>>>>> deny rules)
>>>>>> -     * are ignored in path walks.
>>>>>> -     */
>>>>>> -    if (!path_beneath_attr.allowed_access) {
>>>>>> -        err = -ENOMSG;
>>>>>> -        goto out_put_ruleset;
>>>>>> -    }
>>>>>> -    /*
>>>>>> -     * Checks that allowed_access matches the @ruleset constraints
>>>>>> -     * (ruleset->fs_access_masks[0] is automatically upgraded to 
>>>>>> 64-bits).
>>>>>> -     */
>>>>>> -    if ((path_beneath_attr.allowed_access | 
>>>>>> ruleset->fs_access_masks[0]) !=
>>>>>> -            ruleset->fs_access_masks[0]) {
>>>>>> -        err = -EINVAL;
>>>>>> -        goto out_put_ruleset;
>>>>>> +    switch (rule_type) {
>>>>>> +    case LANDLOCK_RULE_PATH_BENEATH:
>>>>>> +        /* Copies raw user space buffer, for fs rule type. */
>>>>>> +        res = copy_from_user(&path_beneath_attr, rule_attr,
>>>>>> +                    sizeof(path_beneath_attr));
>>>>>> +        if (res)
>>>>>> +            return -EFAULT;
>>>>>> +        break;
>>>>>> +
>>>>>> +    case LANDLOCK_RULE_NET_SERVICE:
>>>>>> +        /* Copies raw user space buffer, for net rule type. */
>>>>>> +        res = copy_from_user(&net_service_attr, rule_attr,
>>>>>> +                sizeof(net_service_attr));
>>>>>> +        if (res)
>>>>>> +            return -EFAULT;
>>>>>> +        break;
>>>>>>       }
>>>>>> -    /* Gets and checks the new rule. */
>>>>>> -    err = get_path_from_fd(path_beneath_attr.parent_fd, &path);
>>>>>> -    if (err)
>>>>>> -        goto out_put_ruleset;
>>>>>> +    if (rule_type == LANDLOCK_RULE_PATH_BENEATH) {
>>>>>> +        /* Gets and checks the ruleset. */
>>>>>> +        ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
>>>>>> +        if (IS_ERR(ruleset))
>>>>>> +            return PTR_ERR(ruleset);
>>>>>> +
>>>>>> +        /*
>>>>>> +         * Informs about useless rule: empty allowed_access (i.e. 
>>>>>> deny rules)
>>>>>> +         * are ignored in path walks.
>>>>>> +         */
>>>>>> +        if (!path_beneath_attr.allowed_access) {
>>>>>> +            err = -ENOMSG;
>>>>>> +            goto out_put_ruleset;
>>>>>> +        }
>>>>>> +        /*
>>>>>> +         * Checks that allowed_access matches the @ruleset 
>>>>>> constraints
>>>>>> +         * (ruleset->access_masks[0] is automatically upgraded to 
>>>>>> 64-bits).
>>>>>> +         */
>>>>>> +        if ((path_beneath_attr.allowed_access | 
>>>>>> ruleset->access_masks[0]) !=
>>>>>> +                            ruleset->access_masks[0]) {
>>>>>> +            err = -EINVAL;
>>>>>> +            goto out_put_ruleset;
>>>>>> +        }
>>>>>> +
>>>>>> +        /* Gets and checks the new rule. */
>>>>>> +        err = get_path_from_fd(path_beneath_attr.parent_fd, &path);
>>>>>> +        if (err)
>>>>>> +            goto out_put_ruleset;
>>>>>> +
>>>>>> +        /* Imports the new rule. */
>>>>>> +        err = landlock_append_fs_rule(ruleset, &path,
>>>>>> +                path_beneath_attr.allowed_access);
>>>>>> +        path_put(&path);
>>>>>> +    }
>>>>>> -    /* Imports the new rule. */
>>>>>> -    err = landlock_append_fs_rule(ruleset, &path,
>>>>>> -            path_beneath_attr.allowed_access);
>>>>>> -    path_put(&path);
>>>>>> +    if (rule_type == LANDLOCK_RULE_NET_SERVICE) {
>>>>>> +        /* Gets and checks the ruleset. */
>>>>>> +        ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
>>>>>
>>>>> You need to factor out more code.
>>>>
>>>>    Sorry. I did not get you here. Please could you explain more 
>>>> detailed?
>>>
>>> Instead of duplicating similar function calls (e.g. 
>>> get_ruleset_from_fd) or operations, try to use one switch statement 
>>> where you put the checks that are different (you can move the 
>>> copy_from_user(&path_beneath_attr...) call). It may be a good idea to 
>>> split this function into 3: one handling each rule_attr, which 
>>> enables to not mix different attr types in the same function. A 
>>> standalone patch should be refactoring the code to add and use a new 
>>> function add_rule_path_beneath(ruleset, rule_attr) (only need the 
>>> "landlock_" prefix for exported functions).
>>
>>    Sorry again. Still don't get the point. What function do you suggetst
>>    to split in 3? Can you please give detailed template of these
>>    functions and the logic?
> 
> You can split SYSCALL_DEFINE4(landlock_add_rule) in 3:
> - a lighten version of SYSCALL_DEFINE4(landlock_add_rule) containing 
> switch cases for rule_type (almost what you did but with the 
> get_ruleset_from_fd moved before);
> - a new add_rule_path_beneath(ruleset, rule_attr) which will be called 
> by the switch case;
> - a new add_rule_net_service(ruleset, rule_attr) which will be called by 
> the switch case.

  Got your point here. Thnak you for the details.
> .

Powered by blists - more mailing lists