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

["Mickaël Salaün" <mic@digikod.net>]

On Thu, Apr 18, 2024 at 02:21:49PM +0200, Günther Noack wrote:
> 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.

OK

> 
> 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.)

I'm OK with your approach as long as the tests are deterministic,
whatever FD 0 is (or not), and as long at they don't have an impact on
FD 0.  To make it more generic and to avoid side effects, I think we
should (always) close FD 0 in ioctl_error() (and explain the reason).

> 
> 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, ...).

Yes, it looks good.

> 
> —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)
>