| Message ID | 20240405214040.101396-9-gnoack@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Landlock: IOCTL support | expand |
On Fri, Apr 05, 2024 at 09:40:36PM +0000, Günther Noack wrote: > This test checks all IOCTL commands implemented in do_vfs_ioctl(). > > Suggested-by: Mickaël Salaün <mic@digikod.net> > Signed-off-by: Günther Noack <gnoack@google.com> > --- > tools/testing/selftests/landlock/fs_test.c | 95 ++++++++++++++++++++++ > 1 file changed, 95 insertions(+) > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > index 10b29a288e9c..e4ba149cf6fd 100644 > --- a/tools/testing/selftests/landlock/fs_test.c > +++ b/tools/testing/selftests/landlock/fs_test.c > @@ -10,6 +10,7 @@ > #define _GNU_SOURCE > #include <asm/termbits.h> > #include <fcntl.h> > +#include <linux/fiemap.h> > #include <linux/landlock.h> > #include <linux/magic.h> > #include <sched.h> > @@ -3937,6 +3938,100 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl) > ASSERT_EQ(0, close(fd)); > } > > +/* > + * ioctl_error - generically call the given ioctl with a pointer to a > + * sufficiently large memory region > + * > + * Returns the IOCTLs error, or 0. > + */ > +static int ioctl_error(int fd, unsigned int cmd) > +{ > + char buf[1024]; /* sufficiently large */ Could we shrink a bit this buffer? > + int res = ioctl(fd, cmd, &buf); > + > + if (res < 0) > + return errno; > + > + return 0; > +} > + > +/* Define some linux/falloc.h IOCTL commands which are not available in uapi headers. */ > +struct space_resv { > + __s16 l_type; > + __s16 l_whence; > + __s64 l_start; > + __s64 l_len; /* len == 0 means until end of file */ > + __s32 l_sysid; > + __u32 l_pid; > + __s32 l_pad[4]; /* reserved area */ > +}; > + > +#define FS_IOC_RESVSP _IOW('X', 40, struct space_resv) > +#define FS_IOC_UNRESVSP _IOW('X', 41, struct space_resv) > +#define FS_IOC_RESVSP64 _IOW('X', 42, struct space_resv) > +#define FS_IOC_UNRESVSP64 _IOW('X', 43, struct space_resv) > +#define FS_IOC_ZERO_RANGE _IOW('X', 57, struct space_resv) > + > +/* > + * Tests a series of blanket-permitted and denied IOCTLs. > + */ > +TEST_F_FORK(layout1, blanket_permitted_ioctls) > +{ > + const struct landlock_ruleset_attr attr = { > + .handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV, > + }; > + int ruleset_fd, fd; > + > + /* Enables Landlock. */ > + ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0); > + ASSERT_LE(0, ruleset_fd); > + enforce_ruleset(_metadata, ruleset_fd); > + ASSERT_EQ(0, close(ruleset_fd)); > + > + fd = open("/dev/null", O_RDWR | O_CLOEXEC); > + ASSERT_LE(0, fd); > + > + /* > + * 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. > + > + /* > + * Checks blocked commands. > + * A call to a blocked IOCTL command always returns EACCES. > + */ > + EXPECT_EQ(EACCES, ioctl_error(fd, FIONREAD)); > + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_GETFLAGS)); > + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_SETFLAGS)); > + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_FSGETXATTR)); > + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_FSSETXATTR)); > + EXPECT_EQ(EACCES, ioctl_error(fd, FIBMAP)); > + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_RESVSP)); > + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_RESVSP64)); > + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_UNRESVSP)); > + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_UNRESVSP64)); > + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_ZERO_RANGE)); Good! > + > + /* Default case is also blocked. */ > + EXPECT_EQ(EACCES, ioctl_error(fd, 0xc00ffeee)); > + > + ASSERT_EQ(0, close(fd)); > +} > + > /* > * Named pipes are not governed by the LANDLOCK_ACCESS_FS_IOCTL_DEV right, > * because they are not character or block devices. > -- > 2.44.0.478.gd926399ef9-goog >
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)
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) >
On Thu, Apr 18, 2024 at 10:44:43PM -0700, Mickaël Salaün wrote: > On Thu, Apr 18, 2024 at 02:21:49PM +0200, Günther Noack wrote: > > 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). Done, good idea. —Günther
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 10b29a288e9c..e4ba149cf6fd 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -10,6 +10,7 @@ #define _GNU_SOURCE #include <asm/termbits.h> #include <fcntl.h> +#include <linux/fiemap.h> #include <linux/landlock.h> #include <linux/magic.h> #include <sched.h> @@ -3937,6 +3938,100 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl) ASSERT_EQ(0, close(fd)); } +/* + * ioctl_error - generically call the given ioctl with a pointer to a + * sufficiently large memory region + * + * Returns the IOCTLs error, or 0. + */ +static int ioctl_error(int fd, unsigned int cmd) +{ + char buf[1024]; /* sufficiently large */ + int res = ioctl(fd, cmd, &buf); + + if (res < 0) + return errno; + + return 0; +} + +/* Define some linux/falloc.h IOCTL commands which are not available in uapi headers. */ +struct space_resv { + __s16 l_type; + __s16 l_whence; + __s64 l_start; + __s64 l_len; /* len == 0 means until end of file */ + __s32 l_sysid; + __u32 l_pid; + __s32 l_pad[4]; /* reserved area */ +}; + +#define FS_IOC_RESVSP _IOW('X', 40, struct space_resv) +#define FS_IOC_UNRESVSP _IOW('X', 41, struct space_resv) +#define FS_IOC_RESVSP64 _IOW('X', 42, struct space_resv) +#define FS_IOC_UNRESVSP64 _IOW('X', 43, struct space_resv) +#define FS_IOC_ZERO_RANGE _IOW('X', 57, struct space_resv) + +/* + * Tests a series of blanket-permitted and denied IOCTLs. + */ +TEST_F_FORK(layout1, blanket_permitted_ioctls) +{ + const struct landlock_ruleset_attr attr = { + .handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV, + }; + int ruleset_fd, fd; + + /* Enables Landlock. */ + ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0); + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + fd = open("/dev/null", O_RDWR | O_CLOEXEC); + ASSERT_LE(0, fd); + + /* + * 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)); + + /* + * Checks blocked commands. + * A call to a blocked IOCTL command always returns EACCES. + */ + EXPECT_EQ(EACCES, ioctl_error(fd, FIONREAD)); + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_GETFLAGS)); + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_SETFLAGS)); + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_FSGETXATTR)); + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_FSSETXATTR)); + EXPECT_EQ(EACCES, ioctl_error(fd, FIBMAP)); + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_RESVSP)); + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_RESVSP64)); + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_UNRESVSP)); + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_UNRESVSP64)); + EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_ZERO_RANGE)); + + /* Default case is also blocked. */ + EXPECT_EQ(EACCES, ioctl_error(fd, 0xc00ffeee)); + + ASSERT_EQ(0, close(fd)); +} + /* * Named pipes are not governed by the LANDLOCK_ACCESS_FS_IOCTL_DEV right, * because they are not character or block devices.
This test checks all IOCTL commands implemented in do_vfs_ioctl(). Suggested-by: Mickaël Salaün <mic@digikod.net> Signed-off-by: Günther Noack <gnoack@google.com> --- tools/testing/selftests/landlock/fs_test.c | 95 ++++++++++++++++++++++ 1 file changed, 95 insertions(+)