Re: [PATCH RFC 3/3] selftests/landlock: Test LANDLOCK_ADD_RULE_CREATED

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/landlock: tautological EINVAL checks in created_rule_invalid()
--

commit 0bd817b616774ee644b06b2a2368b4843876b252
Author: Farid Zakaria <farid.m.zakaria@gmail.com>

selftests/landlock: Test LANDLOCK_ADD_RULE_CREATED

This commit adds Landlock filesystem selftests covering the
LANDLOCK_ADD_RULE_CREATED flag, including newly created entries, renamed
files, nested domains, independent domains, and invalid rule arguments.

> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 7e25a6f6073c0..84980d426436b 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -2640,6 +2640,233 @@ TEST_F_FORK(layout1, refer_mount_root_deny)

[ ... ]

> +/*
> + * Checks that LANDLOCK_ADD_RULE_CREATED is rejected on a file, combined with
> + * LANDLOCK_ADD_RULE_QUIET, or with an empty allowed_access.
> + */
> +TEST_F_FORK(layout1, created_rule_invalid)
> +{
> +	const int ruleset_fd = create_ruleset(_metadata, ACCESS_CREATED, NULL);
> +	struct landlock_path_beneath_attr path_beneath = {
> +		.allowed_access = ACCESS_RW,
> +	};
> +
> +	path_beneath.parent_fd = open(file1_s1d1, O_PATH | O_CLOEXEC);
> +	ASSERT_LE(0, path_beneath.parent_fd);
> +	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
> +					&path_beneath,
> +					LANDLOCK_ADD_RULE_CREATED));
> +	ASSERT_EQ(EINVAL, errno);
> +	ASSERT_EQ(0, close(path_beneath.parent_fd));
> +
> +	path_beneath.parent_fd = open(dir_s1d1, O_PATH | O_CLOEXEC);
> +	ASSERT_LE(0, path_beneath.parent_fd);
> +	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
> +					&path_beneath,
> +					LANDLOCK_ADD_RULE_CREATED |
> +						LANDLOCK_ADD_RULE_QUIET));
> +	ASSERT_EQ(EINVAL, errno);

[Severity: Medium]
Do these two checks in created_rule_invalid() actually exercise the
LANDLOCK_ADD_RULE_CREATED validation paths?

In the first check on file1_s1d1, path_beneath.allowed_access is set to
ACCESS_RW, which includes the directory-only right
LANDLOCK_ACCESS_FS_READ_DIR:

tools/testing/selftests/landlock/fs_test.c:
    #define ACCESS_RO ( \
        LANDLOCK_ACCESS_FS_READ_FILE | \
        LANDLOCK_ACCESS_FS_READ_DIR)

    #define ACCESS_RW ( \
        ACCESS_RO | \
        LANDLOCK_ACCESS_FS_WRITE_FILE)

In security/landlock/fs.c:landlock_append_fs_rule(), a rule on a regular
file is rejected with -EINVAL whenever access_rights is not a subset of
ACCESS_FILE:

    if (!d_is_dir(path->dentry) &&
        ((flags & LANDLOCK_ADD_RULE_CREATED) ||
         !access_mask_subset(access_rights, ACCESS_FILE)))
            return -EINVAL;

Because ACCESS_RW is not a subset of ACCESS_FILE, landlock_add_rule() on
file1_s1d1 will return -1 with errno set to EINVAL even without
LANDLOCK_ADD_RULE_CREATED.

In the second check on dir_s1d1, ruleset_fd is created via
create_ruleset(), which leaves quiet_access_fs as 0. In
security/landlock/syscalls.c:add_rule_path_beneath(), passing
LANDLOCK_ADD_RULE_QUIET on a ruleset with quiet_access.fs == 0 also
returns -EINVAL:

    if (flags & LANDLOCK_ADD_RULE_QUIET && !ruleset->quiet_access.fs)
            return -EINVAL;

Would it make sense to use file-only access rights (such as
LANDLOCK_ACCESS_FS_READ_FILE | LANDLOCK_ACCESS_FS_WRITE_FILE) for the
file1_s1d1 check and a ruleset with quiet_access_fs configured for the
LANDLOCK_ADD_RULE_QUIET check so that both calls would succeed if not for
LANDLOCK_ADD_RULE_CREATED?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-landlock-created-rights-v1-0-77c8db3f6bd2@gmail.com?part=3