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:

quoted

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.

quoted

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.

quoted

Signed-off-by: Konstantin Meskhidze <redacted>
---

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.

quoted

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

quoted

      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

.