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