Re: [PATCH v3 2/5] selftests/landlock: Test ioctl support

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

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