On 19/05/2022 11:23, Konstantin Meskhidze wrote:
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: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.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.Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@xxxxxxxxxx> --- 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.
The syscall argument check ordering needs to stay in the same order as you can see in the add_rule_checks_ordering test. Other than that, this commit looks good, it just splits the syscall in two functions, which is useful.
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.cindex 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).
Right, these tests are correct and they can help you.
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.
