diff mbox series

[v14,03/12] selftests/landlock: Test IOCTL support

Message ID 20240405214040.101396-4-gnoack@google.com (mailing list archive)
State Handled Elsewhere
Headers show
Series Landlock: IOCTL support | expand

Commit Message

Günther Noack April 5, 2024, 9:40 p.m. UTC
Exercises Landlock's IOCTL feature in different combinations of
handling and permitting the LANDLOCK_ACCESS_FS_IOCTL_DEV right, and in
different combinations of using files and directories.

Signed-off-by: Günther Noack <gnoack@google.com>
---
 tools/testing/selftests/landlock/fs_test.c | 227 ++++++++++++++++++++-
 1 file changed, 224 insertions(+), 3 deletions(-)

Comments

Mickaël Salaün April 12, 2024, 3:17 p.m. UTC | #1
On Fri, Apr 05, 2024 at 09:40:31PM +0000, Günther Noack wrote:
> Exercises Landlock's IOCTL feature in different combinations of
> handling and permitting the LANDLOCK_ACCESS_FS_IOCTL_DEV right, and in
> different combinations of using files and directories.
> 
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  tools/testing/selftests/landlock/fs_test.c | 227 ++++++++++++++++++++-
>  1 file changed, 224 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 418ad745a5dd..8a72e26d4977 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c

> +TEST_F_FORK(ioctl, handle_dir_access_file)
> +{
> +	const int flag = 0;
> +	const struct rule rules[] = {
> +		{
> +			.path = "/dev",
> +			.access = variant->allowed,
> +		},
> +		{},
> +	};
> +	int file_fd, ruleset_fd;
> +
> +	/* Enables Landlock. */
> +	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	file_fd = open("/dev/tty", variant->open_mode);

Why /dev/tty? Could we use /dev/null or something less tied to the
current context and less sensitive?

> +	ASSERT_LE(0, file_fd);
> +
> +	/* Checks that IOCTL commands return the expected errors. */
> +	EXPECT_EQ(variant->expected_tcgets_result, test_tcgets_ioctl(file_fd));
> +	EXPECT_EQ(variant->expected_fionread_result,
> +		  test_fionread_ioctl(file_fd));
> +
> +	/* Checks that unrestrictable commands are unrestricted. */
> +	EXPECT_EQ(0, ioctl(file_fd, FIOCLEX));
> +	EXPECT_EQ(0, ioctl(file_fd, FIONCLEX));
> +	EXPECT_EQ(0, ioctl(file_fd, FIONBIO, &flag));
> +	EXPECT_EQ(0, ioctl(file_fd, FIOASYNC, &flag));
> +	EXPECT_EQ(0, ioctl(file_fd, FIGETBSZ, &flag));
> +
> +	ASSERT_EQ(0, close(file_fd));
> +}
> +
> +TEST_F_FORK(ioctl, handle_dir_access_dir)
> +{
> +	const int flag = 0;
> +	const struct rule rules[] = {
> +		{
> +			.path = "/dev",
> +			.access = variant->allowed,
> +		},
> +		{},
> +	};
> +	int dir_fd, ruleset_fd;
> +
> +	/* Enables Landlock. */
> +	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/*
> +	 * Ignore variant->open_mode for this test, as we intend to open a
> +	 * directory.  If the directory can not be opened, the variant is
> +	 * infeasible to test with an opened directory.
> +	 */
> +	dir_fd = open("/dev", O_RDONLY);
> +	if (dir_fd < 0)
> +		return;
> +
> +	/*
> +	 * Checks that IOCTL commands return the expected errors.
> +	 * We do not use the expected values from the fixture here.
> +	 *
> +	 * When using IOCTL on a directory, no Landlock restrictions apply.
> +	 * TCGETS will fail anyway because it is not invoked on a TTY device.
> +	 */
> +	EXPECT_EQ(ENOTTY, test_tcgets_ioctl(dir_fd));
> +	EXPECT_EQ(0, test_fionread_ioctl(dir_fd));
> +
> +	/* Checks that unrestrictable commands are unrestricted. */
> +	EXPECT_EQ(0, ioctl(dir_fd, FIOCLEX));
> +	EXPECT_EQ(0, ioctl(dir_fd, FIONCLEX));
> +	EXPECT_EQ(0, ioctl(dir_fd, FIONBIO, &flag));
> +	EXPECT_EQ(0, ioctl(dir_fd, FIOASYNC, &flag));
> +	EXPECT_EQ(0, ioctl(dir_fd, FIGETBSZ, &flag));
> +
> +	ASSERT_EQ(0, close(dir_fd));
> +}
> +
> +TEST_F_FORK(ioctl, handle_file_access_file)
> +{
> +	const int flag = 0;
> +	const struct rule rules[] = {
> +		{
> +			.path = "/dev/tty0",

Same here (and elsewhere), /dev/null or a similar harmless device file
would be better.

> +			.access = variant->allowed,
> +		},
> +		{},
> +	};
> +	int file_fd, ruleset_fd;
> +
> +	if (variant->allowed & LANDLOCK_ACCESS_FS_READ_DIR) {
> +		SKIP(return, "LANDLOCK_ACCESS_FS_READ_DIR "
> +			     "can not be granted on files");

Can we avoid using SKIP() in this case?  Tests should only be skipped
when not supported, in other words, we should be able to run all tests
with the right combination of architecture and kernel options.

> +	}
> +
> +	/* Enables Landlock. */
> +	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	file_fd = open("/dev/tty0", variant->open_mode);
> +	ASSERT_LE(0, file_fd)
> +	{
> +		TH_LOG("Failed to open /dev/tty0: %s", strerror(errno));
> +	}
> +
> +	/* Checks that IOCTL commands return the expected errors. */
> +	EXPECT_EQ(variant->expected_tcgets_result, test_tcgets_ioctl(file_fd));
> +	EXPECT_EQ(variant->expected_fionread_result,
> +		  test_fionread_ioctl(file_fd));
> +
> +	/* Checks that unrestrictable commands are unrestricted. */
> +	EXPECT_EQ(0, ioctl(file_fd, FIOCLEX));
> +	EXPECT_EQ(0, ioctl(file_fd, FIONCLEX));
> +	EXPECT_EQ(0, ioctl(file_fd, FIONBIO, &flag));
> +	EXPECT_EQ(0, ioctl(file_fd, FIOASYNC, &flag));
> +	EXPECT_EQ(0, ioctl(file_fd, FIGETBSZ, &flag));
> +
> +	ASSERT_EQ(0, close(file_fd));
> +}
> +
>  /* clang-format off */
>  FIXTURE(layout1_bind) {};
>  /* clang-format on */
> -- 
> 2.44.0.478.gd926399ef9-goog
> 
>
Günther Noack April 18, 2024, 11:10 a.m. UTC | #2
On Fri, Apr 12, 2024 at 05:17:44PM +0200, Mickaël Salaün wrote:
> On Fri, Apr 05, 2024 at 09:40:31PM +0000, Günther Noack wrote:
> > Exercises Landlock's IOCTL feature in different combinations of
> > handling and permitting the LANDLOCK_ACCESS_FS_IOCTL_DEV right, and in
> > different combinations of using files and directories.
> > 
> > Signed-off-by: Günther Noack <gnoack@google.com>
> > ---
> >  tools/testing/selftests/landlock/fs_test.c | 227 ++++++++++++++++++++-
> >  1 file changed, 224 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > index 418ad745a5dd..8a72e26d4977 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c
> 
> > +TEST_F_FORK(ioctl, handle_dir_access_file)
> > +{
> > +	const int flag = 0;
> > +	const struct rule rules[] = {
> > +		{
> > +			.path = "/dev",
> > +			.access = variant->allowed,
> > +		},
> > +		{},
> > +	};
> > +	int file_fd, ruleset_fd;
> > +
> > +	/* Enables Landlock. */
> > +	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> > +	ASSERT_LE(0, ruleset_fd);
> > +	enforce_ruleset(_metadata, ruleset_fd);
> > +	ASSERT_EQ(0, close(ruleset_fd));
> > +
> > +	file_fd = open("/dev/tty", variant->open_mode);
> 
> Why /dev/tty? Could we use /dev/null or something less tied to the
> current context and less sensitive?

Absolutely, good point -- I'm switching to /dev/zero.

I am dropping the TCGETS test and only keeping FIONREAD,
but these two IOCTL commands work the same way as far as Landlock is concerned.


> > +TEST_F_FORK(ioctl, handle_file_access_file)
> > +{
> > +	const int flag = 0;
> > +	const struct rule rules[] = {
> > +		{
> > +			.path = "/dev/tty0",
> 
> Same here (and elsewhere), /dev/null or a similar harmless device file
> would be better.

Ditto, /dev/zero it is.


> > +	if (variant->allowed & LANDLOCK_ACCESS_FS_READ_DIR) {
> > +		SKIP(return, "LANDLOCK_ACCESS_FS_READ_DIR "
> > +			     "can not be granted on files");
> 
> Can we avoid using SKIP() in this case?  Tests should only be skipped
> when not supported, in other words, we should be able to run all tests
> with the right combination of architecture and kernel options.

Good point.  After double checking, I realized that this was left over from an
earlier test where we still had more fixture variants.  I'm removing that check.


Thanks for the review!
—Günther
Mickaël Salaün April 19, 2024, 5:44 a.m. UTC | #3
On Fri, Apr 05, 2024 at 09:40:31PM +0000, Günther Noack wrote:
> Exercises Landlock's IOCTL feature in different combinations of
> handling and permitting the LANDLOCK_ACCESS_FS_IOCTL_DEV right, and in
> different combinations of using files and directories.
> 
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  tools/testing/selftests/landlock/fs_test.c | 227 ++++++++++++++++++++-
>  1 file changed, 224 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 418ad745a5dd..8a72e26d4977 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c

> @@ -3831,6 +3842,16 @@ TEST_F_FORK(ftruncate, open_and_ftruncate_in_different_processes)
>  	ASSERT_EQ(0, close(socket_fds[1]));
>  }
>  
> +/* Invokes the FS_IOC_GETFLAGS IOCTL and returns its errno or 0. */
> +static int test_fs_ioc_getflags_ioctl(int fd)
> +{
> +	uint32_t flags;
> +
> +	if (ioctl(fd, FS_IOC_GETFLAGS, &flags) < 0)
> +		return errno;
> +	return 0;
> +}

test_fs_ioc_getflags_ioctl() should be moved to the next patch where it
is used.
Günther Noack April 19, 2024, 2:06 p.m. UTC | #4
On Thu, Apr 18, 2024 at 10:44:00PM -0700, Mickaël Salaün wrote:
> On Fri, Apr 05, 2024 at 09:40:31PM +0000, Günther Noack wrote:
> > +/* Invokes the FS_IOC_GETFLAGS IOCTL and returns its errno or 0. */
> > +static int test_fs_ioc_getflags_ioctl(int fd)
> > +{
> > +	uint32_t flags;
> > +
> > +	if (ioctl(fd, FS_IOC_GETFLAGS, &flags) < 0)
> > +		return errno;
> > +	return 0;
> > +}
> 
> test_fs_ioc_getflags_ioctl() should be moved to the next patch where it
> is used.

Thanks, done.
diff mbox series

Patch

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 418ad745a5dd..8a72e26d4977 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -8,6 +8,7 @@ 
  */
 
 #define _GNU_SOURCE
+#include <asm/termbits.h>
 #include <fcntl.h>
 #include <linux/landlock.h>
 #include <linux/magic.h>
@@ -15,6 +16,7 @@ 
 #include <stdio.h>
 #include <string.h>
 #include <sys/capability.h>
+#include <sys/ioctl.h>
 #include <sys/mount.h>
 #include <sys/prctl.h>
 #include <sys/sendfile.h>
@@ -23,6 +25,12 @@ 
 #include <sys/vfs.h>
 #include <unistd.h>
 
+/*
+ * Intentionally included last to work around header conflict.
+ * See https://sourceware.org/glibc/wiki/Synchronizing_Headers.
+ */
+#include <linux/fs.h>
+
 #include "common.h"
 
 #ifndef renameat2
@@ -737,6 +745,9 @@  static int create_ruleset(struct __test_metadata *const _metadata,
 	}
 
 	for (i = 0; rules[i].path; i++) {
+		if (!rules[i].access)
+			continue;
+
 		add_path_beneath(_metadata, ruleset_fd, rules[i].access,
 				 rules[i].path);
 	}
@@ -3445,7 +3456,7 @@  TEST_F_FORK(layout1, truncate_unhandled)
 			      LANDLOCK_ACCESS_FS_WRITE_FILE;
 	int ruleset_fd;
 
-	/* Enable Landlock. */
+	/* Enables Landlock. */
 	ruleset_fd = create_ruleset(_metadata, handled, rules);
 
 	ASSERT_LE(0, ruleset_fd);
@@ -3528,7 +3539,7 @@  TEST_F_FORK(layout1, truncate)
 			      LANDLOCK_ACCESS_FS_TRUNCATE;
 	int ruleset_fd;
 
-	/* Enable Landlock. */
+	/* Enables Landlock. */
 	ruleset_fd = create_ruleset(_metadata, handled, rules);
 
 	ASSERT_LE(0, ruleset_fd);
@@ -3754,7 +3765,7 @@  TEST_F_FORK(ftruncate, open_and_ftruncate)
 	};
 	int fd, ruleset_fd;
 
-	/* Enable Landlock. */
+	/* Enables Landlock. */
 	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
 	ASSERT_LE(0, ruleset_fd);
 	enforce_ruleset(_metadata, ruleset_fd);
@@ -3831,6 +3842,16 @@  TEST_F_FORK(ftruncate, open_and_ftruncate_in_different_processes)
 	ASSERT_EQ(0, close(socket_fds[1]));
 }
 
+/* Invokes the FS_IOC_GETFLAGS IOCTL and returns its errno or 0. */
+static int test_fs_ioc_getflags_ioctl(int fd)
+{
+	uint32_t flags;
+
+	if (ioctl(fd, FS_IOC_GETFLAGS, &flags) < 0)
+		return errno;
+	return 0;
+}
+
 TEST(memfd_ftruncate)
 {
 	int fd;
@@ -3847,6 +3868,206 @@  TEST(memfd_ftruncate)
 	ASSERT_EQ(0, close(fd));
 }
 
+static int test_fionread_ioctl(int fd)
+{
+	size_t sz = 0;
+
+	if (ioctl(fd, FIONREAD, &sz) < 0 && errno == EACCES)
+		return errno;
+	return 0;
+}
+
+/* clang-format off */
+FIXTURE(ioctl) {};
+
+FIXTURE_SETUP(ioctl) {};
+
+FIXTURE_TEARDOWN(ioctl) {};
+/* clang-format on */
+
+FIXTURE_VARIANT(ioctl)
+{
+	const __u64 handled;
+	const __u64 allowed;
+	const mode_t open_mode;
+	/*
+	 * TCGETS is used as a characteristic device-specific IOCTL command.
+	 * The logic is the same for other IOCTL commands as well.
+	 */
+	const int expected_tcgets_result; /* terminal device IOCTL */
+	/*
+	 * FIONREAD is implemented in fs/ioctl.c for regular files,
+	 * but we do not blanket-permit it for devices.
+	 */
+	const int expected_fionread_result;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_i_allowed_none) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+	.allowed = 0,
+	.open_mode = O_RDWR,
+	.expected_tcgets_result = EACCES,
+	.expected_fionread_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_i_allowed_i) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+	.allowed = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+	.open_mode = O_RDWR,
+	.expected_tcgets_result = 0,
+	.expected_fionread_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, unhandled) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_EXECUTE,
+	.allowed = LANDLOCK_ACCESS_FS_EXECUTE,
+	.open_mode = O_RDWR,
+	.expected_tcgets_result = 0,
+	.expected_fionread_result = 0,
+};
+
+static int test_tcgets_ioctl(int fd)
+{
+	struct termios info;
+
+	if (ioctl(fd, TCGETS, &info) < 0)
+		return errno;
+	return 0;
+}
+
+TEST_F_FORK(ioctl, handle_dir_access_file)
+{
+	const int flag = 0;
+	const struct rule rules[] = {
+		{
+			.path = "/dev",
+			.access = variant->allowed,
+		},
+		{},
+	};
+	int file_fd, ruleset_fd;
+
+	/* Enables Landlock. */
+	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	file_fd = open("/dev/tty", variant->open_mode);
+	ASSERT_LE(0, file_fd);
+
+	/* Checks that IOCTL commands return the expected errors. */
+	EXPECT_EQ(variant->expected_tcgets_result, test_tcgets_ioctl(file_fd));
+	EXPECT_EQ(variant->expected_fionread_result,
+		  test_fionread_ioctl(file_fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(file_fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(file_fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(file_fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(file_fd, FIOASYNC, &flag));
+	EXPECT_EQ(0, ioctl(file_fd, FIGETBSZ, &flag));
+
+	ASSERT_EQ(0, close(file_fd));
+}
+
+TEST_F_FORK(ioctl, handle_dir_access_dir)
+{
+	const int flag = 0;
+	const struct rule rules[] = {
+		{
+			.path = "/dev",
+			.access = variant->allowed,
+		},
+		{},
+	};
+	int dir_fd, ruleset_fd;
+
+	/* Enables Landlock. */
+	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/*
+	 * Ignore variant->open_mode for this test, as we intend to open a
+	 * directory.  If the directory can not be opened, the variant is
+	 * infeasible to test with an opened directory.
+	 */
+	dir_fd = open("/dev", O_RDONLY);
+	if (dir_fd < 0)
+		return;
+
+	/*
+	 * Checks that IOCTL commands return the expected errors.
+	 * We do not use the expected values from the fixture here.
+	 *
+	 * When using IOCTL on a directory, no Landlock restrictions apply.
+	 * TCGETS will fail anyway because it is not invoked on a TTY device.
+	 */
+	EXPECT_EQ(ENOTTY, test_tcgets_ioctl(dir_fd));
+	EXPECT_EQ(0, test_fionread_ioctl(dir_fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(dir_fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(dir_fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(dir_fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(dir_fd, FIOASYNC, &flag));
+	EXPECT_EQ(0, ioctl(dir_fd, FIGETBSZ, &flag));
+
+	ASSERT_EQ(0, close(dir_fd));
+}
+
+TEST_F_FORK(ioctl, handle_file_access_file)
+{
+	const int flag = 0;
+	const struct rule rules[] = {
+		{
+			.path = "/dev/tty0",
+			.access = variant->allowed,
+		},
+		{},
+	};
+	int file_fd, ruleset_fd;
+
+	if (variant->allowed & LANDLOCK_ACCESS_FS_READ_DIR) {
+		SKIP(return, "LANDLOCK_ACCESS_FS_READ_DIR "
+			     "can not be granted on files");
+	}
+
+	/* Enables Landlock. */
+	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	file_fd = open("/dev/tty0", variant->open_mode);
+	ASSERT_LE(0, file_fd)
+	{
+		TH_LOG("Failed to open /dev/tty0: %s", strerror(errno));
+	}
+
+	/* Checks that IOCTL commands return the expected errors. */
+	EXPECT_EQ(variant->expected_tcgets_result, test_tcgets_ioctl(file_fd));
+	EXPECT_EQ(variant->expected_fionread_result,
+		  test_fionread_ioctl(file_fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(file_fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(file_fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(file_fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(file_fd, FIOASYNC, &flag));
+	EXPECT_EQ(0, ioctl(file_fd, FIGETBSZ, &flag));
+
+	ASSERT_EQ(0, close(file_fd));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */