diff mbox series

[v10,6/9] selftests/landlock: Test IOCTLs on named pipes

Message ID 20240309075320.160128-7-gnoack@google.com (mailing list archive)
State New, archived
Headers show
Series Landlock: IOCTL support | expand

Commit Message

Günther Noack March 9, 2024, 7:53 a.m. UTC
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(-)

Comments

Mickaël Salaün March 22, 2024, 7:48 a.m. UTC | #1
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
> 
>
Mickaël Salaün March 22, 2024, 8:45 a.m. UTC | #2
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.
Günther Noack March 22, 2024, 2:39 p.m. UTC | #3
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
Mickaël Salaün March 22, 2024, 3:04 p.m. UTC | #4
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 mbox series

Patch

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;