@@ -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.
@@ -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));
}
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@digikod.net> Link: https://lore.kernel.org/r/20220506160820.524344-9-mic@digikod.net --- 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(-)