diff mbox series

[v4,5/5] Add landlock06 test

Message ID 20240725-landlock-v4-5-66f5a1c0c693@suse.com
State Changes Requested
Headers show
Series landlock testing suite | expand

Commit Message

Andrea Cervesato July 25, 2024, 9:23 a.m. UTC 
From: Andrea Cervesato <andrea.cervesato@suse.com>

This test verifies LANDLOCK_ACCESS_FS_IOCTL_DEV access in the
landlock sandbox by creating a pipe and testing that ioctl() can
be executed on it. The test is also verifying that some of the I/O
operations can be always executed no matter the sandbox rules.
This feature is available since kernel 6.10.

Reviewed-by: Li Wang <liwang@redhat.com>
Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 runtest/syscalls                                |   1 +
 testcases/kernel/syscalls/landlock/.gitignore   |   1 +
 testcases/kernel/syscalls/landlock/landlock06.c | 112 ++++++++++++++++++++++++
 3 files changed, 114 insertions(+)

Comments

Petr Vorel July 26, 2024, 1:24 p.m. UTC | #1 
Hi Andrea,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Few notes below.

...
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.min_kver = "6.10",
nit: would not be able to check the availablity via landlock ABI == 5?

> +	.needs_tmpdir = 1,
> +	.needs_root = 1,
> +	.forks_child = 1,
> +	.needs_kconfigs = (const char *[]) {
> +		"CONFIG_SECURITY_LANDLOCK=y",
> +		NULL
Maybe we don't need this in any landlock test, if we are checking
verify_landlock_is_enabled(), right?

> +	},
> +	.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),
> +		{}
> +	},
> +	.format_device = 1,
> +	.mount_device = 1,
Please remove these two before merge:

$ cd metadata && make
...
testcases/kernel/syscalls/landlock/landlock06.c: useless tag: format_device
testcases/kernel/syscalls/landlock/landlock06.c: useless tag: needs_tmpdir

> +	.mntpoint = MNTPOINT,
> +	.all_filesystems = 1,
> +	.skip_filesystems = (const char *[]) {
> +		"vfat",
> +		"exfat",
I wonder why landlock is not supported...

> +		NULL
> +	},
> +};

Kind regards,
Petr
Mickaël Salaün July 26, 2024, 1:51 p.m. UTC | #2 
On Fri, Jul 26, 2024 at 03:24:40PM +0200, Petr Vorel wrote:
> Hi Andrea,
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> Few notes below.
> 
> ...
> > +static struct tst_test test = {
> > +	.test_all = run,
> > +	.setup = setup,
> > +	.cleanup = cleanup,
> > +	.min_kver = "6.10",
> nit: would not be able to check the availablity via landlock ABI == 5?

Because Landlock is available since 5.13, I guess min_kver should be set
to the same version.
Petr Vorel July 26, 2024, 2:32 p.m. UTC | #3 
> On Fri, Jul 26, 2024 at 03:24:40PM +0200, Petr Vorel wrote:
> > Hi Andrea,

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > Few notes below.

> > ...
> > > +static struct tst_test test = {
> > > +	.test_all = run,
> > > +	.setup = setup,
> > > +	.cleanup = cleanup,
> > > +	.min_kver = "6.10",
> > nit: would not be able to check the availablity via landlock ABI == 5?

> Because Landlock is available since 5.13, I guess min_kver should be set
> to the same version.

If anybody backport this, kernel version will not match. IMHO it's better to
avoid specifying version if we can detect with something else (ABI version in
this case). Also avoid kernel config reading if not needed is IMHO better.

Kind regards,
Petr
Petr Vorel July 26, 2024, 4:16 p.m. UTC | #4 
> > On Fri, Jul 26, 2024 at 03:24:40PM +0200, Petr Vorel wrote:
> > > Hi Andrea,

> > > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > > Few notes below.

> > > ...
> > > > +static struct tst_test test = {
> > > > +	.test_all = run,
> > > > +	.setup = setup,
> > > > +	.cleanup = cleanup,
> > > > +	.min_kver = "6.10",
> > > nit: would not be able to check the availablity via landlock ABI == 5?

> > Because Landlock is available since 5.13, I guess min_kver should be set
> > to the same version.

> If anybody backport this, kernel version will not match. IMHO it's better to
> avoid specifying version if we can detect with something else (ABI version in
> this case). Also avoid kernel config reading if not needed is IMHO better.

Although functionality is not backported often to the enterprise kernels (e.g.
SLES, RHEL), certain things are backported, e.g. [1]. And that is the reason why
LTP even has a way to detect enterprise kernel [2]. Obviously the easiest thing
is to avoid kernel version if there is way to detect functionality by different
way.

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/commit/c354ba291da3e255c135ef78da0c7b8c5556da07
[2] https://github.com/linux-test-project/ltp/blob/master/lib/tst_kvercmp.c#L131

> Kind regards,
> Petr
Petr Vorel July 26, 2024, 4:21 p.m. UTC | #5 
Hi all,

...
> > +	.mntpoint = MNTPOINT,
> > +	.all_filesystems = 1,
> > +	.skip_filesystems = (const char *[]) {
> > +		"vfat",
> > +		"exfat",
> I wonder why landlock is not supported...

It was probably obvious for others:

not only ioctl(file_fd, FIONREAD, &sz), but even mkfifo(sandbox/fifo, 0640)
fails on exfat and vfat.

Kind regards,
Petr

> > +		NULL
> > +	},
> > +};

> Kind regards,
> Petr
diff mbox series

Patch

diff --git a/runtest/syscalls b/runtest/syscalls
index 6522f5bc7..7ebdb41d8 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -701,6 +701,7 @@  landlock02 landlock02
 landlock03 landlock03
 landlock04 landlock04
 landlock05 landlock05
+landlock06 landlock06
 
 lchown01 lchown01
 lchown01_16 lchown01_16
diff --git a/testcases/kernel/syscalls/landlock/.gitignore b/testcases/kernel/syscalls/landlock/.gitignore
index a7ea6be2e..315ac1dca 100644
--- a/testcases/kernel/syscalls/landlock/.gitignore
+++ b/testcases/kernel/syscalls/landlock/.gitignore
@@ -4,3 +4,4 @@  landlock02
 landlock03
 landlock04
 landlock05
+landlock06
diff --git a/testcases/kernel/syscalls/landlock/landlock06.c b/testcases/kernel/syscalls/landlock/landlock06.c
new file mode 100644
index 000000000..647ebbe48
--- /dev/null
+++ b/testcases/kernel/syscalls/landlock/landlock06.c
@@ -0,0 +1,112 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * This test verifies LANDLOCK_ACCESS_FS_IOCTL_DEV access in the
+ * landlock sandbox by creating a pipe and testing that ioctl() can be executed
+ * on it. The test is also verifying that some of the I/O operations can be
+ * always executed no matter the sandbox rules.
+ */
+
+#include "landlock_common.h"
+#include <sys/ioctl.h>
+
+#define MNTPOINT "sandbox"
+#define FILENAME MNTPOINT"/fifo"
+
+static struct landlock_ruleset_attr *ruleset_attr;
+static struct landlock_path_beneath_attr *path_beneath_attr;
+static int file_fd;
+static int dev_fd;
+
+static void run(void)
+{
+	if (SAFE_FORK())
+		return;
+
+	int flag;
+	size_t sz = 0;
+
+	TST_EXP_PASS(ioctl(file_fd, FIONREAD, &sz));
+
+	/* check unrestrictable commands */
+	TST_EXP_PASS(ioctl(dev_fd, FIOCLEX));
+	TST_EXP_PASS(ioctl(dev_fd, FIONCLEX));
+	TST_EXP_PASS(ioctl(dev_fd, FIONBIO, &flag));
+	TST_EXP_PASS(ioctl(dev_fd, FIOASYNC, &flag));
+
+	_exit(0);
+}
+
+static void setup(void)
+{
+	int ruleset_fd;
+
+	verify_landlock_is_enabled();
+
+	SAFE_MKFIFO(FILENAME, 0640);
+
+	file_fd = SAFE_OPEN(FILENAME, O_RDONLY | O_NONBLOCK, 0640);
+	dev_fd = SAFE_OPEN("/dev/zero", O_RDONLY | O_NONBLOCK, 0640);
+
+	tst_res(TINFO, "Applying LANDLOCK_ACCESS_FS_IOCTL_DEV");
+
+	ruleset_attr->handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV;
+
+	ruleset_fd = SAFE_LANDLOCK_CREATE_RULESET(
+		ruleset_attr, sizeof(struct landlock_ruleset_attr), 0);
+
+	apply_landlock_layer(
+		ruleset_attr,
+		path_beneath_attr,
+		MNTPOINT,
+		LANDLOCK_ACCESS_FS_IOCTL_DEV
+	);
+
+	SAFE_CLOSE(ruleset_fd);
+}
+
+static void cleanup(void)
+{
+	if (dev_fd != -1)
+		SAFE_CLOSE(dev_fd);
+
+	if (file_fd != -1)
+		SAFE_CLOSE(file_fd);
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.cleanup = cleanup,
+	.min_kver = "6.10",
+	.needs_tmpdir = 1,
+	.needs_root = 1,
+	.forks_child = 1,
+	.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),
+		{}
+	},
+	.format_device = 1,
+	.mount_device = 1,
+	.mntpoint = MNTPOINT,
+	.all_filesystems = 1,
+	.skip_filesystems = (const char *[]) {
+		"vfat",
+		"exfat",
+		NULL
+	},
+};