[LTP] [PATCH v4 3/5] Add landlock04 test
Petr Vorel
pvorel@suse.cz
Fri Jul 26 15:06:38 CEST 2024
Hi Andrea,
unfortunately one minor things: cleanup needed to fix running with -i
(obviously on all filesystems):
LTP_SINGLE_FS_TYPE=btrfs ./landlock04 -i2 2>&1 |grep TFAIL
landlock_tester.h:206: TFAIL: rmdir(DIR_RMDIR) failed: ENOENT (2)
landlock_tester.h:216: TFAIL: unlink(FILE_UNLINK) failed: ENOENT (2)
landlock_tester.h:217: TFAIL: remove(FILE_REMOVE) failed: ENOENT (2)
landlock_tester.h:233: TFAIL: mknod(path, type | 0400, dev) failed: EEXIST (17)
landlock_tester.h:233: TFAIL: mknod(path, type | 0400, dev) failed: EEXIST (17)
landlock_tester.h:233: TFAIL: mknod(path, type | 0400, dev) failed: EEXIST (17)
landlock_tester.h:233: TFAIL: mknod(path, type | 0400, dev) failed: EEXIST (17)
landlock_tester.h:233: TFAIL: mknod(path, type | 0400, dev) failed: EEXIST (17)
landlock_tester.h:243: TFAIL: symlink(FILE_SYM0, FILE_SYM1) failed: EEXIST (17)
_test_symbolic() and other test functions in landlock_tester.h need to unlink()
after creating files.
TL;DR
The test itself LGTM. Interesting, quite extensive testing, thanks for that!
With:
* fixing on -i
* replaced TST_TO_STR_() instead of ACCESS_NAME()
* lower .max_runtime (or none .max_runtime)
* removing .format_device = 1 and .needs_tmpdir = 1
* add #define LANDLOCK_TESTER_H + append '__' to the definition
you can merge with my:
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Some notes below.
...
> +++ b/testcases/kernel/syscalls/landlock/landlock04.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This test verifies that all landlock filesystem rules are working properly.
> + * The way we do it is to verify that all disabled syscalls are not working but
> + * the one we enabled via specifc landlock rules.
very nit (ignore): I would avoid "This" (bogus word) and use passive form "we do
it". If you look on generated docs, it'd be nice to use a bit consistent style.
But it's minor (the least important thing).
> + */
> +
> +#include "landlock_common.h"
> +#include "landlock_tester.h"
> +#include "tst_safe_stdio.h"
> +
> +#define ACCESS_NAME(x) #x
We have TST_TO_STR_() for this, could you please use it?
> +
> +static struct landlock_ruleset_attr *ruleset_attr;
> +static struct landlock_path_beneath_attr *path_beneath_attr;
> +
> +static struct tvariant {
> + int access;
> + char *desc;
> +} tvariants[] = {
> + {
> + LANDLOCK_ACCESS_FS_READ_FILE | LANDLOCK_ACCESS_FS_EXECUTE,
> + ACCESS_NAME(LANDLOCK_ACCESS_FS_EXECUTE)
s/ACCESS_NAME/TST_TO_STR_/
> + },
...
> +};
...
> +static void enable_exec_libs(const int ruleset_fd)
> +{
> + FILE *fp;
> + char line[1024];
> + char path[PATH_MAX];
> + char dependency[8][PATH_MAX];
> + int count = 0;
> + int duplicate = 0;
> +
> + fp = SAFE_FOPEN("/proc/self/maps", "r");
> +
> + while (fgets(line, sizeof(line), fp)) {
> + if (strstr(line, ".so") == NULL)
> + continue;
> +
> + SAFE_SSCANF(line, "%*x-%*x %*s %*x %*s %*d %s", path);
> +
> + for (int i = 0; i < count; i++) {
> + if (strcmp(path, dependency[i]) == 0) {
> + duplicate = 1;
> + break;
> + }
> + }
> +
> + if (duplicate) {
> + duplicate = 0;
> + continue;
> + }
> +
> + strncpy(dependency[count], path, PATH_MAX);
> + count++;
> +
> + tst_res(TINFO, "Enable read/exec permissions for %s", path);
> +
> + path_beneath_attr->allowed_access =
> + LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_EXECUTE;
> + path_beneath_attr->parent_fd = SAFE_OPEN(path, O_PATH | O_CLOEXEC);
> +
> + SAFE_LANDLOCK_ADD_RULE(
> + ruleset_fd,
> + LANDLOCK_RULE_PATH_BENEATH,
> + path_beneath_attr,
> + 0);
very nit (ignore): for me would be more readable:
SAFE_LANDLOCK_ADD_RULE(
ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, path_beneath_attr, 0);
The same applies to landlock_tester.h below:
static void _test_make(
const char *path,
const int type,
const int dev,
const int result)
> +
> + SAFE_CLOSE(path_beneath_attr->parent_fd);
> + }
> +
> + SAFE_FCLOSE(fp);
> +}
> +
> +static void setup(void)
> +{
> + struct tvariant variant = tvariants[tst_variant];
> + int ruleset_fd;
> +
> + verify_landlock_is_enabled();
> + tester_create_fs_tree();
> +
> + tst_res(TINFO, "Testing %s", variant.desc);
> +
> + ruleset_attr->handled_access_fs = tester_get_all_fs_rules();
> +
> + ruleset_fd = SAFE_LANDLOCK_CREATE_RULESET(
> + ruleset_attr, sizeof(struct landlock_ruleset_attr), 0);
> +
> + /* since our binary is dynamically linked, we need to enable dependences
> + * to be read and executed
> + */
> + enable_exec_libs(ruleset_fd);
> +
> + path_beneath_attr->allowed_access = variant.access;
> + path_beneath_attr->parent_fd = SAFE_OPEN(
> + SANDBOX_FOLDER, O_PATH | O_CLOEXEC);
> +
> + SAFE_LANDLOCK_ADD_RULE(
> + ruleset_fd,
> + LANDLOCK_RULE_PATH_BENEATH,
> + path_beneath_attr,
> + 0);
> +
> + SAFE_CLOSE(path_beneath_attr->parent_fd);
> +
> + enforce_ruleset(ruleset_fd);
> + SAFE_CLOSE(ruleset_fd);
> +}
> +
> +static struct tst_test test = {
> + .test_all = run,
> + .setup = setup,
> + .min_kver = "5.13",
> + .forks_child = 1,
> + .needs_tmpdir = 1,
If you generate docs, you will see:
testcases/kernel/syscalls/landlock/landlock04.c: useless tag: format_device
testcases/kernel/syscalls/landlock/landlock04.c: useless tag: needs_tmpdir
Please remove them (implied by .all_filesystems and others).
> + .needs_root = 1,
> + .test_variants = ARRAY_SIZE(tvariants),
> + .resource_files = (const char *[]) {
> + TESTAPP,
> + NULL,
> + },
> + .needs_kconfigs = (const char *[]) {
> + "CONFIG_SECURITY_LANDLOCK=y",
> + NULL
> + },
> + .bufs = (struct tst_buffers []) {
> + {&ruleset_attr, .size = sizeof(struct landlock_ruleset_attr)},
> + {&path_beneath_attr, .size = sizeof(struct landlock_path_beneath_attr)},
> + {},
> + },
> + .caps = (struct tst_cap []) {
> + TST_CAP(TST_CAP_REQ, CAP_SYS_ADMIN),
> + TST_CAP(TST_CAP_REQ, CAP_MKNOD),
> + {}
> + },
> + .format_device = 1,
> + .mount_device = 1,
> + .mntpoint = SANDBOX_FOLDER,
> + .all_filesystems = 1,
> + .skip_filesystems = (const char *[]) {
> + "vfat",
> + "exfat",
Interesting, ntfs over FUSE works.
> + NULL
> + },
> + .max_runtime = 3600,
Is it really needed for test to run 1 hour? On random VM with 6.5 kernel it runs
with the default (which is 30s).
> +};
> diff --git a/testcases/kernel/syscalls/landlock/landlock_exec.c b/testcases/kernel/syscalls/landlock/landlock_exec.c
> new file mode 100644
> index 000000000..aae5c76b2
> --- /dev/null
> +++ b/testcases/kernel/syscalls/landlock/landlock_exec.c
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> + */
> +
> +int main(void)
> +{
> + return 0;
> +}
> diff --git a/testcases/kernel/syscalls/landlock/landlock_tester.h b/testcases/kernel/syscalls/landlock/landlock_tester.h
> new file mode 100644
> index 000000000..b4a4c1f7d
> --- /dev/null
> +++ b/testcases/kernel/syscalls/landlock/landlock_tester.h
> @@ -0,0 +1,343 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> + */
> +
> +#ifndef LANDLOCK_TESTER_H
You need to have here definition:
#define LANDLOCK_TESTER_H
otherwise the guarder does not work.
While at it, could you please rename it to LANDLOCK_TESTER_H__
(we usually append '__' to avoid clashes with random headers?
See my other fix: 2d24a286f1e35fe6ad7e1e307b86375658e23bee.
> +
> +#include "tst_test.h"
> +#include "lapi/landlock.h"
> +#include <sys/sysmacros.h>
> +
> +#define PERM_MODE 0700
> +
> +#define SANDBOX_FOLDER "sandbox"
> +#define TESTAPP "landlock_exec"
> +
> +#define FILE_EXEC SANDBOX_FOLDER"/"TESTAPP
> +#define FILE_READ SANDBOX_FOLDER"/file_read"
> +#define FILE_WRITE SANDBOX_FOLDER"/file_write"
> +#define FILE_REMOVE SANDBOX_FOLDER"/file_remove"
> +#define FILE_UNLINK SANDBOX_FOLDER"/file_unlink"
> +#define FILE_UNLINKAT SANDBOX_FOLDER"/file_unlinkat"
> +#define FILE_TRUNCATE SANDBOX_FOLDER"/file_truncate"
> +#define FILE_REGULAR SANDBOX_FOLDER"/regular0"
> +#define FILE_SOCKET SANDBOX_FOLDER"/socket0"
> +#define FILE_FIFO SANDBOX_FOLDER"/fifo0"
> +#define FILE_SYM0 SANDBOX_FOLDER"/symbolic0"
> +#define FILE_SYM1 SANDBOX_FOLDER"/symbolic1"
> +#define DIR_READDIR SANDBOX_FOLDER"/dir_readdir"
> +#define DIR_RMDIR SANDBOX_FOLDER"/dir_rmdir"
> +#define DEV_CHAR0 SANDBOX_FOLDER"/chardev0"
> +#define DEV_BLK0 SANDBOX_FOLDER"/blkdev0"
> +
> +#define ALL_RULES (\
> + LANDLOCK_ACCESS_FS_EXECUTE | \
> + LANDLOCK_ACCESS_FS_WRITE_FILE | \
> + LANDLOCK_ACCESS_FS_READ_FILE | \
> + LANDLOCK_ACCESS_FS_READ_DIR | \
> + LANDLOCK_ACCESS_FS_REMOVE_DIR | \
> + LANDLOCK_ACCESS_FS_REMOVE_FILE | \
> + LANDLOCK_ACCESS_FS_MAKE_CHAR | \
> + LANDLOCK_ACCESS_FS_MAKE_DIR | \
> + LANDLOCK_ACCESS_FS_MAKE_REG | \
> + LANDLOCK_ACCESS_FS_MAKE_SOCK | \
> + LANDLOCK_ACCESS_FS_MAKE_FIFO | \
> + LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
> + LANDLOCK_ACCESS_FS_MAKE_SYM | \
> + LANDLOCK_ACCESS_FS_REFER | \
> + LANDLOCK_ACCESS_FS_TRUNCATE | \
> + LANDLOCK_ACCESS_FS_IOCTL_DEV)
I see we haven't covered LANDLOCK_ACCESS_NET_BIND_TCP and
LANDLOCK_ACCESS_NET_CONNECT_TCP (which we added into the LAPI header). This is
not complaining, but it'd be good to remember that. I wanted to note that in the
ticket, but we have no landlock ticket, thus I created one:
https://github.com/linux-test-project/ltp/issues/1181
> +
> +static char *readdir_files[] = {
> + DIR_READDIR"/file0",
> + DIR_READDIR"/file1",
> + DIR_READDIR"/file2",
> +};
> +
> +static int dev_chr;
> +static int dev_blk;
> +
> +static int tester_get_all_fs_rules(void)
> +{
> + int abi;
> + int all_rules = ALL_RULES;
> +
> + abi = SAFE_LANDLOCK_CREATE_RULESET(
> + NULL, 0, LANDLOCK_CREATE_RULESET_VERSION);
> +
> + if (abi < 2)
> + all_rules &= ~LANDLOCK_ACCESS_FS_REFER;
> +
> + if (abi < 3)
> + all_rules &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
> +
> + if (abi < 5)
> + all_rules &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
> +
> + return all_rules;
> +}
> +
> +static void tester_create_fs_tree(void)
> +{
> + if (access(SANDBOX_FOLDER, F_OK) == -1)
> + SAFE_MKDIR(SANDBOX_FOLDER, PERM_MODE);
> +
> + /* folders */
> + SAFE_MKDIR(DIR_RMDIR, PERM_MODE);
> + SAFE_MKDIR(DIR_READDIR, PERM_MODE);
> + for (size_t i = 0; i < ARRAY_SIZE(readdir_files); i++)
> + SAFE_TOUCH(readdir_files[i], PERM_MODE, NULL);
> +
> + /* files */
> + tst_fill_file(FILE_READ, 'a', getpagesize(), 1);
> + SAFE_TOUCH(FILE_WRITE, PERM_MODE, NULL);
> + SAFE_TOUCH(FILE_REMOVE, PERM_MODE, NULL);
> + SAFE_TOUCH(FILE_UNLINK, PERM_MODE, NULL);
> + SAFE_TOUCH(FILE_UNLINKAT, PERM_MODE, NULL);
> + SAFE_TOUCH(FILE_TRUNCATE, PERM_MODE, NULL);
> + SAFE_TOUCH(FILE_SYM0, PERM_MODE, NULL);
> + SAFE_CP(TESTAPP, FILE_EXEC);
> +
> + /* devices */
> + dev_chr = makedev(1, 3);
> + dev_blk = makedev(7, 0);
Shouldn't we check makedev result?
i.g.
if (dev_chr != 0 && errno != EEXIST)
tst_brk("can't create dev_chr");
We probably can ignore it, as we don't test that even in new API tests
(fgetxattr02.c, statx01.c). I suppose it's unlikely to fail (only permissions),
but maybe in the future we should add SAFE_MAKEDEV().
> +}
> +
> +static void _test_exec(const int result)
> +{
> + int status;
> + pid_t pid;
> + char *const args[] = {(char *)FILE_EXEC, NULL};
> +
> + tst_res(TINFO, "Test binary execution");
> +
> + pid = SAFE_FORK();
> + if (!pid) {
> + int rval;
> +
> + if (result == TPASS) {
> + rval = execve(FILE_EXEC, args, NULL);
> + if (rval == -1)
> + tst_res(TFAIL | TERRNO, "Failed to execute test binary");
> + } else {
> + TST_EXP_FAIL(execve(FILE_EXEC, args, NULL), EACCES);
> + }
> +
> + _exit(1);
> + }
> +
> + SAFE_WAITPID(pid, &status, 0);
> + if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
Why don't we TFAIL on result == TPASS? What am I missing?
> + return;
> +
> + tst_res(result, "Test binary has been executed");
> +}
> +
> +static void _test_write(const int result)
> +{
> + tst_res(TINFO, "Test writing file");
> +
> + if (result == TPASS)
> + TST_EXP_FD(open(FILE_WRITE, O_WRONLY, PERM_MODE));
> + else
> + TST_EXP_FAIL(open(FILE_WRITE, O_WRONLY, PERM_MODE), EACCES);
> +
> + if (TST_RET != -1)
> + SAFE_CLOSE(TST_RET);
> +}
> +
> +static void _test_read(const int result)
> +{
> + tst_res(TINFO, "Test reading file");
> +
> + if (result == TPASS)
> + TST_EXP_FD(open(FILE_READ, O_RDONLY, PERM_MODE));
> + else
> + TST_EXP_FAIL(open(FILE_READ, O_RDONLY, PERM_MODE), EACCES);
> +
> + if (TST_RET != -1)
> + SAFE_CLOSE(TST_RET);
> +}
> +
> +static void _test_readdir(const int result)
> +{
> + tst_res(TINFO, "Test reading directory");
> +
> + DIR *dir;
> + struct dirent *de;
> + int files_counted = 0;
> +
> + dir = opendir(DIR_READDIR);
> + if (!dir) {
> + tst_res(result == TPASS ? TFAIL : TPASS,
> + "Can't read '%s' directory", DIR_READDIR);
> +
> + return;
> + }
> +
> + tst_res(result, "Can read '%s' directory", DIR_READDIR);
> + if (result == TFAIL)
> + return;
NOTE the check above is needed only if 'dir = opendir(DIR_READDIR);' passes on
result == TFAIL. I'm not sure if this can happen, but probably better to assume it can.
> +
> + while ((de = readdir(dir)) != NULL) {
> + if (de->d_type != DT_REG)
> + continue;
> +
> + for (size_t i = 0; i < ARRAY_SIZE(readdir_files); i++) {
> + if (readdir_files[i] == NULL)
> + continue;
> +
> + if (strstr(readdir_files[i], de->d_name) != NULL)
> + files_counted++;
> + }
> + }
> +
> + SAFE_CLOSEDIR(dir);
> +
> + TST_EXP_EQ_LI(files_counted, ARRAY_SIZE(readdir_files));
> +}
> +
> +static void _test_rmdir(const int result)
> +{
> + tst_res(TINFO, "Test removing directory");
> +
> + if (result == TPASS)
> + TST_EXP_PASS(rmdir(DIR_RMDIR));
> + else
> + TST_EXP_FAIL(rmdir(DIR_RMDIR), EACCES);
> +}
> +
> +static void _test_rmfile(const int result)
> +{
> + tst_res(TINFO, "Test removing file");
> +
> + if (result == TPASS) {
> + TST_EXP_PASS(unlink(FILE_UNLINK));
> + TST_EXP_PASS(remove(FILE_REMOVE));
> + } else {
> + TST_EXP_FAIL(unlink(FILE_UNLINK), EACCES);
> + TST_EXP_FAIL(remove(FILE_REMOVE), EACCES);
> + }
> +}
> +
> +static void _test_make(
> + const char *path,
> + const int type,
> + const int dev,
> + const int result)
...
> +{
> + tst_res(TINFO, "Test normal or special files creation");
> +
> + if (result == TPASS)
> + TST_EXP_PASS(mknod(path, type | 0400, dev));
> + else
> + TST_EXP_FAIL(mknod(path, type | 0400, dev), EACCES);
Here I guess we need to remove file to enable running with -i.
> +}
> +
> +static void _test_symbolic(const int result)
> +{
> + tst_res(TINFO, "Test symbolic links");
> +
> + if (result == TPASS)
> + TST_EXP_PASS(symlink(FILE_SYM0, FILE_SYM1));
> + else
> + TST_EXP_FAIL(symlink(FILE_SYM0, FILE_SYM1), EACCES);
Also here.
> +}
> +
> +static void _test_truncate(const int result)
> +{
> + int fd;
> +
> + tst_res(TINFO, "Test truncating file");
> +
> + if (result == TPASS) {
> + TST_EXP_PASS(truncate(FILE_TRUNCATE, 10));
> +
> + fd = TST_EXP_FD(open(FILE_TRUNCATE, O_WRONLY, PERM_MODE));
Shouldn't we use SAFE_OPEN() here? To quit early on error.
> + if (fd != -1) {
> + TST_EXP_PASS(ftruncate(fd, 10));
> + SAFE_CLOSE(fd);
> + }
> +
> + fd = TST_EXP_FD(open(FILE_TRUNCATE, O_WRONLY | O_TRUNC, PERM_MODE));
> + if (fd != -1)
> + SAFE_CLOSE(fd);
> + } else {
> + TST_EXP_FAIL(truncate(FILE_TRUNCATE, 10), EACCES);
> +
> + fd = open(FILE_TRUNCATE, O_WRONLY, PERM_MODE);
> + if (fd != -1) {
> + TST_EXP_FAIL(ftruncate(fd, 10), EACCES);
> + SAFE_CLOSE(fd);
> + }
> +
> + TST_EXP_FAIL(open(FILE_TRUNCATE, O_WRONLY | O_TRUNC, PERM_MODE),
> + EACCES);
> +
> + if (TST_RET != -1)
> + SAFE_CLOSE(TST_RET);
> + }
> +}
> +
...
The rest LGTM.
Kind regards,
Petr
More information about the ltp
mailing list