This makes more sense to first check the ruleset FD and then the rule attribute. It will be useful to factor out code for other rule types. Add inval_add_rule_arguments tests, extension of empty_path_beneath_attr tests, to also check error ordering for landlock_add_rule(2). Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx> Link: https://lore.kernel.org/r/20220506160820.524344-9-mic@xxxxxxxxxxx --- Changes since v1: * New patch. --- security/landlock/syscalls.c | 22 +++++++------ tools/testing/selftests/landlock/base_test.c | 34 ++++++++++++++++++-- 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c index 7edc1d50e2bf..a7396220c9d4 100644 --- a/security/landlock/syscalls.c +++ b/security/landlock/syscalls.c @@ -318,20 +318,24 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd, if (flags) return -EINVAL; - if (rule_type != LANDLOCK_RULE_PATH_BENEATH) - 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); + 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; + } + /* * Informs about useless rule: empty allowed_access (i.e. deny rules) * are ignored in path walks. diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c index be9b937256ac..18b779471dcb 100644 --- a/tools/testing/selftests/landlock/base_test.c +++ b/tools/testing/selftests/landlock/base_test.c @@ -121,20 +121,50 @@ TEST(inval_create_ruleset_flags) ASSERT_EQ(EINVAL, errno); } -TEST(empty_path_beneath_attr) +/* Tests ordering of syscall argument checks. */ +TEST(add_rule_checks_ordering) { const struct landlock_ruleset_attr ruleset_attr = { .handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE, }; + struct landlock_path_beneath_attr path_beneath_attr = { + .allowed_access = LANDLOCK_ACCESS_FS_EXECUTE, + .parent_fd = -1, + }; const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); ASSERT_LE(0, ruleset_fd); - /* Similar to struct landlock_path_beneath_attr.parent_fd = 0 */ + /* Checks invalid flags. */ + ASSERT_EQ(-1, landlock_add_rule(-1, 0, NULL, 1)); + ASSERT_EQ(EINVAL, errno); + + /* Checks invalid ruleset FD. */ + ASSERT_EQ(-1, landlock_add_rule(-1, 0, NULL, 0)); + ASSERT_EQ(EBADF, errno); + + /* Checks invalid rule type. */ + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, 0, NULL, 0)); + ASSERT_EQ(EINVAL, errno); + + /* Checks invalid rule attr. */ ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, NULL, 0)); ASSERT_EQ(EFAULT, errno); + + /* Checks invalid path_beneath.parent_fd. */ + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, + &path_beneath_attr, 0)); + ASSERT_EQ(EBADF, errno); + + /* Checks valid call. */ + path_beneath_attr.parent_fd = + open("/tmp", O_PATH | O_NOFOLLOW | O_DIRECTORY | O_CLOEXEC); + ASSERT_LE(0, path_beneath_attr.parent_fd); + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, + &path_beneath_attr, 0)); + ASSERT_EQ(0, close(path_beneath_attr.parent_fd)); ASSERT_EQ(0, close(ruleset_fd)); } -- 2.35.1