On Fri, Nov 24, 2023 at 05:57:31PM +0100, Günther Noack wrote:
> Hi!
>
> On Mon, Nov 20, 2023 at 09:41:20PM +0100, Mickaël Salaün wrote:
> > On Fri, Nov 17, 2023 at 04:49:16PM +0100, Günther Noack wrote:
> > > +FIXTURE_VARIANT(ioctl)
> > > +{
> > > + const __u64 handled;
> > > + const __u64 permitted;
> >
> > Why not "allowed" like the rule's field? Same for the variant names.
>
> Just for consistency with the ftruncate tests which also named it like that... %-)
>
> Sounds good though, I'll just rename it in both places.
>
>
> > > + const mode_t open_mode;
> > > + /*
> > > + * These are the expected IOCTL results for a representative IOCTL from
> > > + * each of the IOCTL groups. We only distinguish the 0 and EACCES
> > > + * results here, and treat other errors as 0.
> >
> > In this case, why not use a boolean instead of a semi-correct error
> > code?
>
> I found it slightly less convoluted. When we use booleans here, we need to map
> between error codes and booleans at a different layer. At the same time, we
> already have various test_foo_ioctl() and test_foo() functions, and they are
> sometimes also used in other contexts like test_fs_ioc_getflags_ioctl(). These
> test_foo() helpers generally return error codes so far, and it felt more
> important to stay consistent with that. If we want to keep that both, the only
> other place to map between booleans and error codes would be with ternary
> operators or such in the EXPECT_EQ clauses, but that also felt like it would
> turn unreadable... %-)
Sounds good.
>
> I can change it if you feel strongly about it though. Let me know.
>
> > > + */
> > > + const int expected_fioqsize_result; /* G1 */
> > > + const int expected_fibmap_result; /* G2 */
> > > + const int expected_fionread_result; /* G3 */
> > > + const int expected_fs_ioc_zero_range_result; /* G4 */
> > > + const int expected_fs_ioc_getflags_result; /* other */
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(ioctl, ioctl_handled_i_permitted_none) {
> >
> > You can remove all the variant's "ioctl_" prefixes.
>
> Done.
>
>
> > > + /* clang-format on */
> > > + .handled = LANDLOCK_ACCESS_FS_EXECUTE | LANDLOCK_ACCESS_FS_IOCTL,
> > > + .permitted = LANDLOCK_ACCESS_FS_EXECUTE,
> >
> > You could use 0 instead and don't add the related rule in this case.
>
> Done.
>
>
> > Great tests!
>
> Thanks :)
>
>
> > > +static int test_fioqsize_ioctl(int fd)
> > > +{
> > > + size_t sz;
> > > +
> > > + if (ioctl(fd, FIOQSIZE, &sz) < 0)
> > > + return errno;
> > > + return 0;
> > > +}
> > > +
> > > +static int test_fibmap_ioctl(int fd)
> > > +{
> > > + int blk = 0;
> > > +
> > > + /*
> > > + * We only want to distinguish here whether Landlock already caught it,
> > > + * so we treat anything but EACCESS as success. (It commonly returns
> > > + * EPERM when missing CAP_SYS_RAWIO.)
> > > + */
> > > + if (ioctl(fd, FIBMAP, &blk) < 0 && errno == EACCES)
> > > + return errno;
> > > + return 0;
> > > +}
> > > +
> > > +static int test_fionread_ioctl(int fd)
> > > +{
> > > + size_t sz = 0;
> > > +
> > > + if (ioctl(fd, FIONREAD, &sz) < 0 && errno == EACCES)
> > > + return errno;
> > > + return 0;
> > > +}
> > > +
> > > +#define FS_IOC_ZERO_RANGE _IOW('X', 57, struct space_resv)
> > > +
> > > +static int test_fs_ioc_zero_range_ioctl(int fd)
> > > +{
> > > + 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 */
> > > + } reservation = {};
> > > + /*
> > > + * This can fail for various reasons, but we only want to distinguish
> > > + * here whether Landlock already caught it, so we treat anything but
> > > + * EACCES as success.
> > > + */
> > > + if (ioctl(fd, FS_IOC_ZERO_RANGE, &reservation) < 0 && errno == EACCES)
> >
> > What are the guarantees that an error different than EACCES would not
> > mask EACCES and then make tests pass whereas they should not?
>
> It is indeed possible that one of these ioctls legitimately returns EACCES after
> Landlock was letting that ioctl through, and then we could not tell apart
> whether Landlock blocked it or whether the underlying IOCTL command returned
> that. I double checked that this is not the case for these specific
> invocations. But with some other IOCTL commands in these groups, I believe I
> was getting EACCES sometimes from the IOCTL. So we only use one representative
> IOCTL from each IOCTL group, for which it happened to work.
>
> To convince yourself, you can see in the tests that we have both "success" and
> "blocked" examples in the tests for each of these IOCTL commands, and the
> Landlock rules are the only difference between these examples. Therefore, we
> know that it is actually Landlock returning the EACCES and not the underlying
> IOCTL.
This is correct, at least for now. Let's stick to that.
>
> > > + return errno;
> > > + return 0;
> > > +}
> > > +
> > > +TEST_F_FORK(ioctl, handle_dir_access_file)
> > > +{
> > > + const int flag = 0;
> > > + const struct rule rules[] = {
> > > + {
> > > + .path = dir_s1d1,
> > > + .access = variant->permitted,
> > > + },
> > > + {},
> > > + };
> > > + int fd, ruleset_fd;
> >
> > Please rename fd into something like file_fd.
>
> Done.
>
>
> > > +TEST_F_FORK(ioctl, handle_file_access_file)
> > > +{
> > > + const char *const path = file1_s1d1;
> > > + const int flag = 0;
> > > + const struct rule rules[] = {
> > > + {
> > > + .path = path,
> > > + .access = variant->permitted,
> > > + },
> > > + {},
> > > + };
> > > + int fd, ruleset_fd;
> > > +
> > > + if (variant->permitted & LANDLOCK_ACCESS_FS_READ_DIR) {
> > > + /* This access right can not be granted on files. */
> > > + return;
> > > + }
> >
> > You should use SKIP().
>
> Done.
>
>
> > > + /* Enables Landlock. */
> > > + ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> > > + ASSERT_LE(0, ruleset_fd);
> > > + enforce_ruleset(_metadata, ruleset_fd);
> > > + ASSERT_EQ(0, close(ruleset_fd));
> > > +
> > > + fd = open(path, variant->open_mode);
> > > + ASSERT_LE(0, fd);
> > > +
> > > + /*
> > > + * Checks that IOCTL commands in each IOCTL group return the expected
> > > + * errors.
> > > + */
> > > + EXPECT_EQ(variant->expected_fioqsize_result, test_fioqsize_ioctl(fd));
> > > + EXPECT_EQ(variant->expected_fibmap_result, test_fibmap_ioctl(fd));
> > > + EXPECT_EQ(variant->expected_fionread_result, test_fionread_ioctl(fd));
> > > + EXPECT_EQ(variant->expected_fs_ioc_zero_range_result,
> > > + test_fs_ioc_zero_range_ioctl(fd));
> > > + EXPECT_EQ(variant->expected_fs_ioc_getflags_result,
> > > + test_fs_ioc_getflags_ioctl(fd));
> > > +
> > > + /* Checks that unrestrictable commands are unrestricted. */
> > > + EXPECT_EQ(0, ioctl(fd, FIOCLEX));
> > > + EXPECT_EQ(0, ioctl(fd, FIONCLEX));
> > > + EXPECT_EQ(0, ioctl(fd, FIONBIO, &flag));
> > > + EXPECT_EQ(0, ioctl(fd, FIOASYNC, &flag));
> > > +
> > > + ASSERT_EQ(0, close(fd));
> > > +}
> >
> > Don't you want to create and use a common helper with most of these
> > TEST_F_FORK blocks? It would highlight what is the same or different,
> > and it would also enables to extend the coverage to other file types
> > (e.g. character device).
>
> I did not find a good way to factor this out, to be honest, and so preferred to
> keep it unrolled.
>
> I try to follow the rule of not putting too many "if" conditions and logic into
> my tests (it helps to keep the tests straightforward and understandable), and I
> find it more straightforward to spell out these few EXPECT_EQs three times than
> introducing a "test_all_ioctl_expectations()" helper function :)
I understand, but I'm not sure there would be so much "if" that it would
be difficult to understand. Did you try to factor it out with the
_metadata argument?
>
> —Günther
>
