Re: [PATCH v14 08/12] selftests/landlock: Exhaustive test for the IOCTL allow-list

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

On Fri, Apr 12, 2024 at 05:18:06PM +0200, Mickaël Salaün wrote:
> On Fri, Apr 05, 2024 at 09:40:36PM +0000, Günther Noack wrote:
> > +static int ioctl_error(int fd, unsigned int cmd)
> > +{
> > +	char buf[1024]; /* sufficiently large */
> 
> Could we shrink a bit this buffer?

Shrunk to 128.

I'm also zeroing the buffer now, which was missing before,
to make the behaviour deterministic.


> > +	int res = ioctl(fd, cmd, &buf);
> > +
> > +	if (res < 0)
> > +		return errno;
> > +
> > +	return 0;
> > +}


> > +TEST_F_FORK(layout1, blanket_permitted_ioctls)
> > +{
> > +   [...]
> > +	/*
> > +	 * Checks permitted commands.
> > +	 * These ones may return errors, but should not be blocked by Landlock.
> > +	 */
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIOCLEX));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIONCLEX));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIONBIO));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIOASYNC));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIOQSIZE));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIFREEZE));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FITHAW));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_FIEMAP));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIGETBSZ));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FICLONE));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FICLONERANGE));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIDEDUPERANGE));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_GETFSUUID));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_GETFSSYSFSPATH));
> 
> Could we check for ENOTTY instead of !EACCES? /dev/null should be pretty
> stable.

The expected results are all over the place, unfortunately.
When I tried it, I got this:

        EXPECT_EQ(0, ioctl_error(fd, FIOCLEX));
        EXPECT_EQ(0, ioctl_error(fd, FIONCLEX));
        EXPECT_EQ(0, ioctl_error(fd, FIONBIO));
        EXPECT_EQ(0, ioctl_error(fd, FIOASYNC));
        EXPECT_EQ(ENOTTY, ioctl_error(fd, FIOQSIZE));
        EXPECT_EQ(EPERM, ioctl_error(fd, FIFREEZE));
        EXPECT_EQ(EPERM, ioctl_error(fd, FITHAW));
        EXPECT_EQ(EOPNOTSUPP, ioctl_error(fd, FS_IOC_FIEMAP));
        EXPECT_EQ(0, ioctl_error(fd, FIGETBSZ));
        EXPECT_EQ(EBADF, ioctl_error(fd, FICLONE));
        EXPECT_EQ(EXDEV, ioctl_error(fd, FICLONERANGE));  // <----
        EXPECT_EQ(EINVAL, ioctl_error(fd, FIDEDUPERANGE));
        EXPECT_EQ(0, ioctl_error(fd, FS_IOC_GETFSUUID));
        EXPECT_EQ(ENOTTY, ioctl_error(fd, FS_IOC_GETFSSYSFSPATH));

I find this difficult to read and it distracts from the main point, which
is that we got past the Landlock check which would have returned an EACCES.

I spotted an additional problem with FICLONERANGE -- when we pass a
zero-initialized buffer to that IOCTL, it'll interpret some of these zeros
to refer to file descriptor 0 (stdin)... and what that means is not
controlled by the test - the error code can change depending on what that
FD is.  (I don't want to start filling in all these structs individually.)

The only thing that really matters to us is that the result is not EACCES
(==> we have gotten past the Landlock policy check).  Testing the exact
behaviour of all of these IOCTLs is maybe stepping too much on the turf of
these IOCTL implementations and making the test more brittle towards
cahnges unrelated to Landlock than they need to be [1].

So, if you are OK with that, I would prefer to keep these checks using
EXPECT_NE(EACCES, ...).

—Günther

[1] https://abseil.io/resources/swe-book/html/ch12.html has a discussion on
    why to avoid brittle tests (written about Google, but applicable here
    as well, IMHO)