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 objectsIt'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@xxxxxxxxxxx/Ok. I will add patch vervison in next patch. So it will be "[RFC PATCHv4 ../..] 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@xxxxxxxxxx v2: https://lore.kernel.org/netdev/20211228115212.703084-1-konstantin.meskhidze@xxxxxxxxxx/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.
.
