On Fri, Aug 25, 2023 at 05:51:09PM +0200, Günther Noack wrote:
> Hello!
>
> On Fri, Aug 18, 2023 at 07:06:07PM +0200, Mickaël Salaün wrote:
> > On Mon, Aug 14, 2023 at 07:28:13PM +0200, Günther Noack wrote:
> > > @@ -3639,7 +3639,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
> > > };
> > > int fd, ruleset_fd;
> > >
> > > - /* Enable Landlock. */
> > > + /* Enables Landlock. */
> > > ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> > > ASSERT_LE(0, ruleset_fd);
> > > enforce_ruleset(_metadata, ruleset_fd);
> > > @@ -3732,6 +3732,96 @@ TEST(memfd_ftruncate)
> > > ASSERT_EQ(0, close(fd));
> > > }
> >
> > We should also check with O_PATH to make sure the correct error is
> > returned (and not EACCES).
>
> Is this remark referring to the code before it or after it?
>
> My interpretation is that you are asking to test that test_fioqsize_ioctl() will
> return errnos correctly? Do I understand that correctly? (I think that would
> be a little bit overdone, IMHO - it's just a test utility of ~10 lines after
> all, which is below the threshold where it can be verified by staring at it for
> a bit. :))
I was refering to the previous memfd_ftruncate test, which is changed
with a next patch. We should check the access rights tied (and checkd)
to FD (i.e. truncate and ioctl) opened with O_PATH.
>
> > > +/* Invokes the FIOQSIZE ioctl(2) and returns its errno or 0. */
> > > +static int test_fioqsize_ioctl(int fd)
> > > +{
> > > + loff_t size;
> > > +
> > > + if (ioctl(fd, FIOQSIZE, &size) < 0)
> > > + return errno;
> > > + return 0;
> > > +}
>
>
>
> > > + dir_s1d1_fd = open(dir_s1d1, O_RDONLY);
> >
> > You can use O_CLOEXEC everywhere.
>
> Done.
>
>
> > > + ASSERT_LE(0, dir_s1d1_fd);
> > > + file1_s1d1_fd = open(file1_s1d1, O_RDONLY);
> > > + ASSERT_LE(0, file1_s1d1_fd);
> > > + dir_s2d1_fd = open(dir_s2d1, O_RDONLY);
> > > + ASSERT_LE(0, dir_s2d1_fd);
> > > +
> > > + /*
> > > + * Checks that FIOQSIZE works on files where LANDLOCK_ACCESS_FS_IOCTL is
> > > + * permitted.
> > > + */
> > > + EXPECT_EQ(EACCES, test_fioqsize_ioctl(dir_s1d1_fd));
> > > + EXPECT_EQ(0, test_fioqsize_ioctl(file1_s1d1_fd));
> > > + EXPECT_EQ(0, test_fioqsize_ioctl(dir_s2d1_fd));
> > > +
> > > + /* Closes all file descriptors. */
> > > + ASSERT_EQ(0, close(dir_s1d1_fd));
> > > + ASSERT_EQ(0, close(file1_s1d1_fd));
> > > + ASSERT_EQ(0, close(dir_s2d1_fd));
> > > +}
> > > +
> > > +TEST_F_FORK(layout1, ioctl_always_allowed)
> > > +{
> > > + struct landlock_ruleset_attr attr = {
> >
> > const struct landlock_ruleset_attr attr = {
>
> Done.
>
> I am personally unsure whether "const" is worth it for local variables, but I am
> happy to abide by whatever the dominant style is. (The kernel style guide
> doesn't seem to mention it though.)
I prefer to constify as much as possible to be notified when a write
will be needed for a patch. From a security point of view, it's always
good to have as much as possible read-only data, at least in theory (it
might not always be enforced in memory). It's also useful as
documentation.
>
> BTW, it's somewhat inconsistent within this file already -- we should maybe
> clean this up.
I probably missed some, more constification would be good, but not with
this patch series.
>
>
> > > + .handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL,
> > > + };
> > > + int ruleset_fd, fd;
> > > + int flag = 0;
> > > + int n;
> >
> > const int flag = 0;
> > int ruleset_fd, test_fd, n;
>
> Done.
>
> Thanks for the review!
> —Günther
>
> --
> Sent using Mutt 🐕 Woof Woof
