| Message ID | 20240405214040.101396-4-gnoack@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Landlock: IOCTL support | expand |
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 > >
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
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.
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 --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 */
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(-)