Re: [LTP] [PATCH v4 3/5] Add landlock04 test

[Petr Vorel <pvorel@suse.cz>]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp