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