Message-ID: <cc023cc4-043b-c67e-1e6e-acf1eb18d155@huawei.com>
Date:   Tue, 8 Feb 2022 10:55:57 +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/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 ?
> 
> [...]
> 
>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>>>> new file mode 100644
>>>> index 000000000000..0b5323d254a7
>>>> --- /dev/null
>>>> +++ b/security/landlock/net.c
>>>> @@ -0,0 +1,175 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Landlock LSM - Filesystem management and hooks
>>>> + *
>>>> + * Copyright © 2016-2020 Mickaël Salaün <mic@...ikod.net>
>>>> + * Copyright © 2018-2020 ANSSI
>>>> + */
>>>> +
>>>> +#include <linux/socket.h>
>>>> +#include <linux/net.h>
>>>> +#include <linux/in.h>
>>>
>>> Why is linux/in.h required?
>>>
>>    Struct sockaddr_in is described in this header.
>>    A pointer to struct sockaddr_in is used in hook_socket_connect()
>>    and hook_socket_bind() to get socket's family and port values.
> 
> OK, good point.
> 
> [...]
> 
>>>> +        return 0;
>>>> +
>>>> +    socket_type = sock->type;
>>>> +    /* Check if it's a TCP socket */
>>>> +    if (socket_type != SOCK_STREAM)
>>>> +        return 0;
>>>> +
>>>> +    if (!dom)
>>>> +        return 0;
>>>
>>> This must be at the top of *each* hook to make it clear that they 
>>> don't impact non-landlocked processes.
>>>
>>    They don't impact. It does not matter what to check first socket
>>    family/type or landlocked process.
> 
> It doesn't change the semantic but it changes the reviewing which is 
> easier with common and consistent sequential checks (and could avoid 
> future mistakes). This rule is followed by all Landlock hooks.

   Ok.
> 
> [...]
> 
>>>> @@ -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?

> [...]
> 
>>>> @@ -142,26 +159,36 @@ static void build_check_ruleset(void)
>>>>    * access rights.
>>>>    */
>>>>   static int insert_rule(struct landlock_ruleset *const ruleset,
>>>> -        struct landlock_object *const object,
>>>> -        const struct landlock_layer (*const layers)[],
>>>> -        size_t num_layers)
>>>> +        void *const obj, const struct landlock_layer (*const 
>>>> layers)[],
>>>
>>> same here
>>       Do you mean using 2 object arguments in insert_rule():
>>
>>      1. insert_rule( object_ptr = landlock_object , object_data = 0,
>>                                 ...,  fs_rule_type);
>>          2. insert_rule( object_ptr = NULL , object_data = port, .... ,
>>                           net_rule_type);
> 
> Yes
> 
> [...]
> 
>>>> @@ -336,9 +399,11 @@ static int inherit_ruleset(struct 
>>>> landlock_ruleset *const parent,
>>>>           err = -EINVAL;
>>>>           goto out_unlock;
>>>>       }
>>>> -    /* Copies the parent layer stack and leaves a space for the new 
>>>> layer. */
>>>> -    memcpy(child->fs_access_masks, parent->fs_access_masks,
>>>> -            flex_array_size(parent, fs_access_masks, 
>>>> parent->num_layers));
>>>> +    /* Copies the parent layer stack and leaves a space for the new 
>>>> layer.
>>>
>>> ditto
>>        Do you mean comments style here?
> 
> Yes
> 
> [...]
> 
>>>> @@ -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?

> .

Powered by blists - more mailing lists