On 08/02/2022 08:55, Konstantin Meskhidze wrote:

2/7/2022 5:17 PM, Mickaël Salaün пишет:

quoted

On 07/02/2022 14:09, Konstantin Meskhidze wrote:

quoted

2/1/2022 3:13 PM, Mickaël Salaün пишет:

quoted

On 24/01/2022 09:02, Konstantin Meskhidze wrote:

quoted

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/ (local)

  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 (local) 

    v2: 
https://lore.kernel.org/netdev/20211228115212.703084-1-konstantin.meskhidze@huawei.com/ (local) 

    v3: ....

  right ?

Absolutely! This is a good practice (and would be better in reverse order).

quoted

[...]

quoted

quoted

quoted

@@ -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)

quoted

[...]

quoted

quoted

quoted

@@ -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.

quoted

quoted

          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.