| Message ID | 20240309075320.160128-7-gnoack@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Landlock: IOCTL support | expand |
It might be interesting to create a layout with one file of each type and use that for the IOCTL tests. On Sat, Mar 09, 2024 at 07:53:17AM +0000, Günther Noack wrote: > Named pipes should behave like pipes created with pipe(2), > so we don't want to restrict IOCTLs on them. > > Suggested-by: Mickaël Salaün <mic@digikod.net> > Signed-off-by: Günther Noack <gnoack@google.com> > --- > tools/testing/selftests/landlock/fs_test.c | 61 ++++++++++++++++++---- > 1 file changed, 52 insertions(+), 9 deletions(-) > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > index 5c47231a722e..d991f44875bc 100644 > --- a/tools/testing/selftests/landlock/fs_test.c > +++ b/tools/testing/selftests/landlock/fs_test.c > @@ -3924,6 +3924,58 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl) > 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; > +} > + > +/* > + * Named pipes are not governed by the LANDLOCK_ACCESS_FS_IOCTL_DEV right, > + * because they are not character or block devices. > + */ > +TEST_F_FORK(layout1, named_pipe_ioctl) > +{ > + pid_t child_pid; > + int fd, ruleset_fd; > + const char *const path = file1_s1d1; > + const struct landlock_ruleset_attr attr = { > + .handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV, > + }; > + > + ASSERT_EQ(0, unlink(path)); > + ASSERT_EQ(0, mkfifo(path, 0600)); > + > + /* Enables Landlock. */ > + ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0); > + ASSERT_LE(0, ruleset_fd); > + enforce_ruleset(_metadata, ruleset_fd); > + ASSERT_EQ(0, close(ruleset_fd)); > + > + /* The child process opens the pipe for writing. */ > + child_pid = fork(); > + ASSERT_NE(-1, child_pid); > + if (child_pid == 0) { What is the purpose of this child's code? > + fd = open(path, O_WRONLY); > + close(fd); > + exit(0); > + } > + > + fd = open(path, O_RDONLY); > + ASSERT_LE(0, fd); > + > + /* FIONREAD is implemented by pipefifo_fops. */ > + EXPECT_EQ(0, test_fionread_ioctl(fd)); > + > + ASSERT_EQ(0, close(fd)); > + ASSERT_EQ(0, unlink(path)); > + > + ASSERT_EQ(child_pid, waitpid(child_pid, NULL, 0)); > +} > + > /* clang-format off */ > FIXTURE(ioctl) {}; > > @@ -3997,15 +4049,6 @@ static int test_tcgets_ioctl(int fd) > return 0; > } > > -static int test_fionread_ioctl(int fd) > -{ > - size_t sz = 0; > - > - if (ioctl(fd, FIONREAD, &sz) < 0 && errno == EACCES) > - return errno; > - return 0; > -} > - You should add test_fionread_ioctl() at the right place from the start. > TEST_F_FORK(ioctl, handle_dir_access_file) > { > const int flag = 0; > -- > 2.44.0.278.ge034bb2e1d-goog > >
On Fri, Mar 22, 2024 at 08:48:30AM +0100, Mickaël Salaün wrote: > It might be interesting to create a layout with one file of each type > and use that for the IOCTL tests. To make sure we only restrict the first layer of IOCTL (handled by the VFS) we should check that an IOCTL command that should be handled by a specific filesystem is indeed passed through this filesystem and not blocked by Landlock. Because Landlock would return EACCES, I guess it should be enough to check that we get a ENOTTY for non-block/char devices. We should find an IOCTL command number that has little chance to be taken to avoid updating this test too often.
On Fri, Mar 22, 2024 at 08:48:29AM +0100, Mickaël Salaün wrote: > It might be interesting to create a layout with one file of each type > and use that for the IOCTL tests. We have already written these tests and we can keep them, but I think that we only gain little additional confidence from testing non-device files. The implementation is saying pretty directly that IOCTLs are permitted if the file is not a character or block device, at the top of the file_ioctl hook. I don't see much value in testing this even more exhaustively and would like to keep it as it is for now. > On Sat, Mar 09, 2024 at 07:53:17AM +0000, Günther Noack wrote: > > Named pipes should behave like pipes created with pipe(2), > > so we don't want to restrict IOCTLs on them. > > > > Suggested-by: Mickaël Salaün <mic@digikod.net> > > Signed-off-by: Günther Noack <gnoack@google.com> > > --- > > tools/testing/selftests/landlock/fs_test.c | 61 ++++++++++++++++++---- > > 1 file changed, 52 insertions(+), 9 deletions(-) > > > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > > index 5c47231a722e..d991f44875bc 100644 > > --- a/tools/testing/selftests/landlock/fs_test.c > > +++ b/tools/testing/selftests/landlock/fs_test.c > > @@ -3924,6 +3924,58 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl) > > 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; > > +} > > + > > +/* > > + * Named pipes are not governed by the LANDLOCK_ACCESS_FS_IOCTL_DEV right, > > + * because they are not character or block devices. > > + */ > > +TEST_F_FORK(layout1, named_pipe_ioctl) > > +{ > > + pid_t child_pid; > > + int fd, ruleset_fd; > > + const char *const path = file1_s1d1; > > + const struct landlock_ruleset_attr attr = { > > + .handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV, > > + }; > > + > > + ASSERT_EQ(0, unlink(path)); > > + ASSERT_EQ(0, mkfifo(path, 0600)); > > + > > + /* Enables Landlock. */ > > + ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0); > > + ASSERT_LE(0, ruleset_fd); > > + enforce_ruleset(_metadata, ruleset_fd); > > + ASSERT_EQ(0, close(ruleset_fd)); > > + > > + /* The child process opens the pipe for writing. */ > > + child_pid = fork(); > > + ASSERT_NE(-1, child_pid); > > + if (child_pid == 0) { > > What is the purpose of this child's code? From fifo(7): Opening the FIFO blocks until the other end is opened also. So the child and parent process both wait for the other open to happen. I suspect I could technically also use O_RDWR here, but that is undefined behaviour in POSIX and less conventional code. (This is described further down, also in fifo(7).) > > > + fd = open(path, O_WRONLY); > > + close(fd); > > + exit(0); > > + } > > + > > + fd = open(path, O_RDONLY); > > + ASSERT_LE(0, fd); > > + > > + /* FIONREAD is implemented by pipefifo_fops. */ > > + EXPECT_EQ(0, test_fionread_ioctl(fd)); > > + > > + ASSERT_EQ(0, close(fd)); > > + ASSERT_EQ(0, unlink(path)); > > + > > + ASSERT_EQ(child_pid, waitpid(child_pid, NULL, 0)); > > +} > > + > > /* clang-format off */ > > FIXTURE(ioctl) {}; > > > > @@ -3997,15 +4049,6 @@ static int test_tcgets_ioctl(int fd) > > return 0; > > } > > > > -static int test_fionread_ioctl(int fd) > > -{ > > - size_t sz = 0; > > - > > - if (ioctl(fd, FIONREAD, &sz) < 0 && errno == EACCES) > > - return errno; > > - return 0; > > -} > > - > > You should add test_fionread_ioctl() at the right place from the start. Fair enough, done. > > TEST_F_FORK(ioctl, handle_dir_access_file) > > { > > const int flag = 0; > > -- > > 2.44.0.278.ge034bb2e1d-goog > > > > —Günther
On Fri, Mar 22, 2024 at 03:39:55PM +0100, Günther Noack wrote: > On Fri, Mar 22, 2024 at 08:48:29AM +0100, Mickaël Salaün wrote: > > It might be interesting to create a layout with one file of each type > > and use that for the IOCTL tests. > > We have already written these tests and we can keep them, but I think that we > only gain little additional confidence from testing non-device files. The > implementation is saying pretty directly that IOCTLs are permitted if the file > is not a character or block device, at the top of the file_ioctl hook. I don't > see much value in testing this even more exhaustively and would like to keep it > as it is for now. OK, let's keep them for now, it's easy to remove a patch/commit.
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 5c47231a722e..d991f44875bc 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -3924,6 +3924,58 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl) 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; +} + +/* + * Named pipes are not governed by the LANDLOCK_ACCESS_FS_IOCTL_DEV right, + * because they are not character or block devices. + */ +TEST_F_FORK(layout1, named_pipe_ioctl) +{ + pid_t child_pid; + int fd, ruleset_fd; + const char *const path = file1_s1d1; + const struct landlock_ruleset_attr attr = { + .handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV, + }; + + ASSERT_EQ(0, unlink(path)); + ASSERT_EQ(0, mkfifo(path, 0600)); + + /* Enables Landlock. */ + ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0); + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + /* The child process opens the pipe for writing. */ + child_pid = fork(); + ASSERT_NE(-1, child_pid); + if (child_pid == 0) { + fd = open(path, O_WRONLY); + close(fd); + exit(0); + } + + fd = open(path, O_RDONLY); + ASSERT_LE(0, fd); + + /* FIONREAD is implemented by pipefifo_fops. */ + EXPECT_EQ(0, test_fionread_ioctl(fd)); + + ASSERT_EQ(0, close(fd)); + ASSERT_EQ(0, unlink(path)); + + ASSERT_EQ(child_pid, waitpid(child_pid, NULL, 0)); +} + /* clang-format off */ FIXTURE(ioctl) {}; @@ -3997,15 +4049,6 @@ static int test_tcgets_ioctl(int fd) return 0; } -static int test_fionread_ioctl(int fd) -{ - size_t sz = 0; - - if (ioctl(fd, FIONREAD, &sz) < 0 && errno == EACCES) - return errno; - return 0; -} - TEST_F_FORK(ioctl, handle_dir_access_file) { const int flag = 0;
Named pipes should behave like pipes created with pipe(2), so we don't want to restrict IOCTLs on them. Suggested-by: Mickaël Salaün <mic@digikod.net> Signed-off-by: Günther Noack <gnoack@google.com> --- tools/testing/selftests/landlock/fs_test.c | 61 ++++++++++++++++++---- 1 file changed, 52 insertions(+), 9 deletions(-)