Re: [PATCH v3 2/5] selftests/landlock: Test ioctl support

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

Hello!

On Fri, Aug 25, 2023 at 07:07:01PM +0200, Mickaël Salaün wrote:
> On Fri, Aug 25, 2023 at 05:51:09PM +0200, Günther Noack wrote:
> > Hello!
> > 
> > On Fri, Aug 18, 2023 at 07:06:07PM +0200, Mickaël Salaün wrote:
> > > On Mon, Aug 14, 2023 at 07:28:13PM +0200, Günther Noack wrote:
> > > > @@ -3639,7 +3639,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
> > > >  	};
> > > >  	int fd, ruleset_fd;
> > > >  
> > > > -	/* Enable Landlock. */
> > > > +	/* Enables Landlock. */
> > > >  	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> > > >  	ASSERT_LE(0, ruleset_fd);
> > > >  	enforce_ruleset(_metadata, ruleset_fd);
> > > > @@ -3732,6 +3732,96 @@ TEST(memfd_ftruncate)
> > > >  	ASSERT_EQ(0, close(fd));
> > > >  }
> > > 
> > > We should also check with O_PATH to make sure the correct error is
> > > returned (and not EACCES).
> > 
> > Is this remark referring to the code before it or after it?
> > 
> > My interpretation is that you are asking to test that test_fioqsize_ioctl() will
> > return errnos correctly?  Do I understand that correctly?  (I think that would
> > be a little bit overdone, IMHO - it's just a test utility of ~10 lines after
> > all, which is below the threshold where it can be verified by staring at it for
> > a bit. :))
> 
> I was refering to the previous memfd_ftruncate test, which is changed
> with a next patch. We should check the access rights tied (and checkd)
> to FD (i.e. truncate and ioctl) opened with O_PATH.

OK, I added a test that checks ioctl(2) and ftruncate(2) on files that
were opened with O_PATH, both before and after enabling Landlock.
ftruncate() and ioctl() always give an EBADF error, both before and
after enabling Landlock (as described in open(2) in the section about
O_PATH).

A bit outside of the IOCTL path set scope:

I was surprised that it is even possible to successfully open a file
with O_PATH, even after Landlock is enabled and restricts all it can
in that file hierarchy.  This lets you detect that a file exists, even
when that file is in a directory whose contents you are otherwise not
permitted to list due to Landlock.

The logic for that is in the get_required_file_open_access() function.
Should we add a "LANDLOCK_ACCESS_FS_PATH_FILE" right, which would work
similar to LANDLOCK_ACCESS_FS_READ_FILE and
LANDLOCK_ACCESS_FS_WRITE_FILE, so that this can be restricted?

—Günther