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

["Mickaël Salaün" <mic@digikod.net>]

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
> 
>