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:

quoted

On Mon, Aug 14, 2023 at 07:28:13PM +0200, Günther Noack wrote:

quoted

@@ -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.

quoted

quoted

+/* 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;
+}

quoted

quoted

+	dir_s1d1_fd = open(dir_s1d1, O_RDONLY);

You can use O_CLOEXEC everywhere.

Done.

quoted

quoted

+	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.

quoted

quoted

+		.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