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

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

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