On 9/27/2024 5:53 PM, Günther Noack wrote:
On Wed, Sep 04, 2024 at 06:48:21PM +0800, Mikhail Ivanov wrote:
Add test validating that socket creation with accept(2) is not restricted
by Landlock.
Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@xxxxxxxxxxxxxxxxxxx>
---
.../testing/selftests/landlock/socket_test.c | 71 +++++++++++++++++++
1 file changed, 71 insertions(+)
diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
index 2ab27196fa3d..052dbe0d1227 100644
--- a/tools/testing/selftests/landlock/socket_test.c
+++ b/tools/testing/selftests/landlock/socket_test.c
@@ -939,4 +939,75 @@ TEST_F(socket_creation, sctp_peeloff)
ASSERT_EQ(0, close(server_fd));
}
+TEST_F(socket_creation, accept)
+{
+ int status;
+ pid_t child;
+ struct sockaddr_in addr;
+ int server_fd, client_fd;
+ char buf;
+ const struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
+ };
+ struct landlock_socket_attr tcp_socket_create = {
^^^^^^
Could be const as well, just like the ruleset_attr?
(I probably overlooked this as well in some of the other tests.)
Yeap, I'll fix this for each test.
+ .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
+ .family = AF_INET,
+ .type = SOCK_STREAM,
+ };
+
+ server_fd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0);
+ ASSERT_LE(0, server_fd);
+
+ addr.sin_family = AF_INET;
+ addr.sin_port = htons(loopback_port);
+ addr.sin_addr.s_addr = inet_addr(loopback_ipv4);
+
+ ASSERT_EQ(0, bind(server_fd, &addr, sizeof(addr)));
+ ASSERT_EQ(0, listen(server_fd, backlog));
+
+ child = fork();
+ ASSERT_LE(0, child);
+ if (child == 0) {
Nit:
I feel like the child code would benefit from a higher level comment,
like "Connects to the server once and exits." or such.
Agreed, I'll add this
+ /* Closes listening socket for the child. */
+ ASSERT_EQ(0, close(server_fd));
+
+ client_fd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0);
+ ASSERT_LE(0, client_fd);
+
+ ASSERT_EQ(0, connect(client_fd, &addr, sizeof(addr)));
+ EXPECT_EQ(1, write(client_fd, ".", 1));
+
+ ASSERT_EQ(0, close(client_fd));
+ _exit(_metadata->exit_code);
+ return;
+ }
+
+ if (self->sandboxed) {
+ int ruleset_fd = landlock_create_ruleset(
+ &ruleset_attr, sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+ if (self->allowed) {
+ ASSERT_EQ(0, landlock_add_rule(ruleset_fd,
+ LANDLOCK_RULE_SOCKET,
+ &tcp_socket_create, 0));
+ }
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+ }
+
+ client_fd = accept(server_fd, NULL, 0);
+
+ /* accept(2) should not be restricted by Landlock. */
+ EXPECT_LE(0, client_fd);
Should be an ASSERT, IMHO.
If this fails, client_fd will be -1,
and a lot of the stuff afterwards will fail as well.
Agreed, thank you!
+
+ EXPECT_EQ(1, read(client_fd, &buf, 1));
+ EXPECT_EQ('.', buf);
I'm torn on whether the "." write and the check for it is very useful in this test.
It muddies the test's purpose a bit, and makes it harder to recognize the main use case.
Might make the test a bit simpler to drop it.
Agreed, this check is really not that important.
+
+ ASSERT_EQ(child, waitpid(child, &status, 0));
+ ASSERT_EQ(1, WIFEXITED(status));
+ ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
+
+ ASSERT_EQ(0, close(server_fd));
You are missing to close client_fd.
will be fixed
+}
+
TEST_HARNESS_MAIN
--
2.34.1
