Re: [RFC 3/4] selftests/landlock: Test ioctl support

["Günther Noack" <gnoack@google.com>]

Hello!

Thanks for the review!

On Mon, Jun 19, 2023 at 04:42:17PM +0200, Mickaël Salaün wrote:
> On 02/05/2023 19:17, Günther Noack wrote:
> > Exercise the use of Landlock's ioctl restriction: If ioctl is
> > restricted, the use of ioctl fails with a freshly opened /dev/tty
> > file.
> > 
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >   tools/testing/selftests/landlock/fs_test.c | 62 ++++++++++++++++++++++
> >   1 file changed, 62 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > index fdd7d439ce4..1f827604374 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c
> > @@ -3655,6 +3655,68 @@ TEST(memfd_ftruncate)
> >   	ASSERT_EQ(0, close(fd));
> >   }
> > +/*
> > + * Invokes ioctl(2) and returns its errno or 0.
> > + * The provided fd needs to be a tty for this to work.
> > + */
> > +static int test_tty_ioctl(int fd)
> > +{
> > +	struct winsize ws;
> > +
> > +	if (ioctl(fd, TIOCGWINSZ, &ws) < 0)
> > +		return errno;
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Attempt ioctl on /dev/tty0 and /dev/tty1,
> > + * with file descriptors opened before and after landlocking.
> > + */
> > +TEST_F_FORK(layout1, ioctl)
> > +{
> > +	const struct rule rules[] = {
> > +		{
> > +			.path = "/dev/tty1",
> > +			.access = LANDLOCK_ACCESS_FS_IOCTL,
> > +		},
> > +		/* Implicitly: No ioctl access on /dev/tty0. */
> 
> We should create a new PTS mount point, create a new session, and use that
> for tests to limit the dependency on the test environment and not mess with
> it.

I have pondered this, and I feel that this is unnecessarily complicating the
test.  The mechanism that I intend to test here is just the general filtering of
IOCTL commands, but not TTYs specifically.  TTYs are a common use case for
IOCTLs, but they are not the only one.

If you are not strongly opposed to it, I would rather look for a different IOCTL
command that works on a different file, where we don't need any special set up?
That would simplify the test and exercise the same functionality in the end.
Does that sounds reasonable?


> > +		{},
> > +	};
> > +	const __u64 handled = LANDLOCK_ACCESS_FS_IOCTL;
> > +	int ruleset_fd;
> > +	int old_tty0_fd, tty0_fd, tty1_fd;
> > +
> > +	old_tty0_fd = open("/dev/tty0", O_RDWR);
> > +	ASSERT_LE(0, old_tty0_fd);
> > +
> > +	/* Checks that ioctl works before landlocking. */
> > +	EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd));
> > +
> > +	/* Enable Landlock. */
> 
> Enable*s*

Done.


> > +	ruleset_fd = create_ruleset(_metadata, handled, rules);
> > +	ASSERT_LE(0, ruleset_fd);
> > +	enforce_ruleset(_metadata, ruleset_fd);
> > +	ASSERT_EQ(0, close(ruleset_fd));
> > +
> > +	/* Checks that ioctl with existing FD works after landlocking. */
> > +	EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd));
> > +
> > +	/* Checks that same ioctl fails when file is opened after landlocking. */
> > +	tty0_fd = open("/dev/tty0", O_RDWR);
> > +	ASSERT_LE(0, tty0_fd);
> > +	EXPECT_EQ(EACCES, test_tty_ioctl(tty0_fd));
> > +
> > +	/* Checks that same ioctl fails when file is opened after landlocking. */
> > +	tty1_fd = open("/dev/tty1", O_RDWR);
> > +	ASSERT_LE(0, tty1_fd);
> > +	EXPECT_EQ(0, test_tty_ioctl(tty1_fd));
> 
> /dev, or rather the test PTS mount point, and its parent, should also be
> tested. We can use three layers in the same test for that.

We've already tested the inheritance of access rights across different
directories and mount points in other tests.  I feel that exercising it in all
combinations of access rights and inheritance mechanisms makes the tests harder
to understand and maintain, and does not give us much additional confidence on
top of what we already have.  What balance do you want to find there?

Thanks,
—Günther

-- 
Sent using Mutt 🐕 Woof Woof