[PATCH] selftests/landlock: Remove invalid unix socket bind()

[PATCH] selftests/landlock: Remove invalid unix socket bind()

[Matthieu Buffet]

Remove bind() call on a client socket that doesn't make sense.
Since strlen(cli_un.sun_path) returns a random value depending on stack
garbage, that many uninitialized bytes are read from the stack as an
unix socket address. This creates random test failures due to the bind
address being invalid or already in use if the same stack value comes up
twice.

Fixes: f83d51a5bdfe ("selftests/landlock: Check IOCTL restrictions for named UNIX domain sockets")
Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
---
 tools/testing/selftests/landlock/fs_test.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index eee814e09dd7..7d378bdf3bce 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -4391,9 +4391,6 @@ TEST_F_FORK(layout1, named_unix_domain_socket_ioctl)
 	cli_fd = socket(AF_UNIX, SOCK_STREAM, 0);
 	ASSERT_LE(0, cli_fd);
 
-	size = offsetof(struct sockaddr_un, sun_path) + strlen(cli_un.sun_path);
-	ASSERT_EQ(0, bind(cli_fd, (struct sockaddr *)&cli_un, size));
-
 	bzero(&cli_un, sizeof(cli_un));
 	cli_un.sun_family = AF_UNIX;
 	strncpy(cli_un.sun_path, path, sizeof(cli_un.sun_path));

base-commit: 54f9baf537b0a091adad860ec92e3e18e0a0754c
-- 
2.47.3

Re: [PATCH] selftests/landlock: Remove invalid unix socket bind()

[Günther Noack]

Hello!

On Mon, Dec 01, 2025 at 01:36:31AM +0100, Matthieu Buffet wrote:
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index eee814e09dd7..7d378bdf3bce 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -4391,9 +4391,6 @@ TEST_F_FORK(layout1, named_unix_domain_socket_ioctl)
>  	cli_fd = socket(AF_UNIX, SOCK_STREAM, 0);
>  	ASSERT_LE(0, cli_fd);
>  
> -	size = offsetof(struct sockaddr_un, sun_path) + strlen(cli_un.sun_path);
> -	ASSERT_EQ(0, bind(cli_fd, (struct sockaddr *)&cli_un, size));
> -
>  	bzero(&cli_un, sizeof(cli_un));
>  	cli_un.sun_family = AF_UNIX;
>  	strncpy(cli_un.sun_path, path, sizeof(cli_un.sun_path));
> 
> base-commit: 54f9baf537b0a091adad860ec92e3e18e0a0754c
> -- 
> 2.47.3
> 

Reviewed-by: Günther Noack <gnoack@google.com>

It looks like I must have fumbled with the copy&paste in that test,
this bind() call does not make sense in the place where it is and is
not necessary for the test.  Apologies for that and thank you for
spotting this!


Optional: In hindsight, I think it would be nice to simplify the way
that we calculate the address length here.  I probably mis-read
unix(7) at the time, where a similar formula is used (but with a
"+1").  Re-reading it, it seems that just passing sizeof(cli_un) as
the address length would have been the simpler and less error prone
solution:

From unix(7) (emphasis mine):

    The addrlen argument that describes the enclosing sockaddr_un
    structure should have a value of at least:

        offsetof(struct sockaddr_un, sun_path)+strlen(addr.sun_path)+1

    or, more simply, addrlen **can be specified as sizeof(struct
    sockaddr_un).**

So, I believe that all the places where we calculate the size in this
function can just disappear and we can directly use sizeof(cli_un) and
sizeof(srv_un) in the bind() and connect() calls instead.

(Let me know whether you want to do it in this change, otherwise I am
also happy to send a follow-up fix.)

Thanks,
—Günther

Re: [PATCH] selftests/landlock: Remove invalid unix socket bind()

[Matthieu Buffet]

Hi Günther,

On 12/2/2025 9:37 AM, Günther Noack wrote:
> Optional: In hindsight, I think it would be nice to simplify the way
> that we calculate the address length here.  I probably mis-read
> unix(7) at the time, where a similar formula is used (but with a
> "+1").  Re-reading it, it seems that just passing sizeof(cli_un) as
> the address length would have been the simpler and less error prone
> solution:

You're right, the rest of the code uses non-NULL-terminated pathnames. 
Not a problem per se on Linux, but these computations make tests less 
readable. I will send a patch based on this fix. Thanks!

Matthieu

[PATCH] selftests/landlock: NULL-terminate unix pathname addresses

[Matthieu Buffet]

The size of Unix pathname addresses is computed in selftests using
offsetof(struct sockaddr_un, sun_path) + strlen(xxx). It should have
been that +1, which makes addresses passed to the libc and kernel
non-NULL-terminated. unix_mkname_bsd() fixes that in Linux so there is
no harm, but just using sizeof(the address struct) should improve
readability.

Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
---
Based on commit https://lore.kernel.org/linux-security-module/20251201003631.190817-1-matthieu@buffet.re/

 tools/testing/selftests/landlock/fs_test.c    | 24 +++++++++----------
 .../landlock/scoped_abstract_unix_test.c      | 21 +++++++---------
 2 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 7d378bdf3bce..76491ba54dce 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -4362,22 +4362,24 @@ TEST_F_FORK(layout1, named_unix_domain_socket_ioctl)
 {
 	const char *const path = file1_s1d1;
 	int srv_fd, cli_fd, ruleset_fd;
-	socklen_t size;
-	struct sockaddr_un srv_un, cli_un;
+	struct sockaddr_un srv_un = {
+		.sun_family = AF_UNIX,
+	};
+	struct sockaddr_un cli_un = {
+		.sun_family = AF_UNIX,
+	};
 	const struct landlock_ruleset_attr attr = {
 		.handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV,
 	};
 
 	/* Sets up a server */
-	srv_un.sun_family = AF_UNIX;
-	strncpy(srv_un.sun_path, path, sizeof(srv_un.sun_path));
-
 	ASSERT_EQ(0, unlink(path));
 	srv_fd = socket(AF_UNIX, SOCK_STREAM, 0);
 	ASSERT_LE(0, srv_fd);
 
-	size = offsetof(struct sockaddr_un, sun_path) + strlen(srv_un.sun_path);
-	ASSERT_EQ(0, bind(srv_fd, (struct sockaddr *)&srv_un, size));
+	strncpy(srv_un.sun_path, path, sizeof(srv_un.sun_path));
+	ASSERT_EQ(0, bind(srv_fd, (struct sockaddr *)&srv_un, sizeof(srv_un)));
+
 	ASSERT_EQ(0, listen(srv_fd, 10 /* qlen */));
 
 	/* Enables Landlock. */
@@ -4387,16 +4389,12 @@ TEST_F_FORK(layout1, named_unix_domain_socket_ioctl)
 	ASSERT_EQ(0, close(ruleset_fd));
 
 	/* Sets up a client connection to it */
-	cli_un.sun_family = AF_UNIX;
 	cli_fd = socket(AF_UNIX, SOCK_STREAM, 0);
 	ASSERT_LE(0, cli_fd);
 
-	bzero(&cli_un, sizeof(cli_un));
-	cli_un.sun_family = AF_UNIX;
 	strncpy(cli_un.sun_path, path, sizeof(cli_un.sun_path));
-	size = offsetof(struct sockaddr_un, sun_path) + strlen(cli_un.sun_path);
-
-	ASSERT_EQ(0, connect(cli_fd, (struct sockaddr *)&cli_un, size));
+	ASSERT_EQ(0,
+		  connect(cli_fd, (struct sockaddr *)&cli_un, sizeof(cli_un)));
 
 	/* FIONREAD and other IOCTLs should not be forbidden. */
 	EXPECT_EQ(0, test_fionread_ioctl(cli_fd));
diff --git a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
index 6825082c079c..2cdf1ba07016 100644
--- a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
+++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
@@ -779,7 +779,6 @@ FIXTURE_TEARDOWN(various_address_sockets)
 
 TEST_F(various_address_sockets, scoped_pathname_sockets)
 {
-	socklen_t size_stream, size_dgram;
 	pid_t child;
 	int status;
 	char buf_child, buf_parent;
@@ -798,12 +797,8 @@ TEST_F(various_address_sockets, scoped_pathname_sockets)
 	/* Pathname address. */
 	snprintf(stream_pathname_addr.sun_path,
 		 sizeof(stream_pathname_addr.sun_path), "%s", stream_path);
-	size_stream = offsetof(struct sockaddr_un, sun_path) +
-		      strlen(stream_pathname_addr.sun_path);
 	snprintf(dgram_pathname_addr.sun_path,
 		 sizeof(dgram_pathname_addr.sun_path), "%s", dgram_path);
-	size_dgram = offsetof(struct sockaddr_un, sun_path) +
-		     strlen(dgram_pathname_addr.sun_path);
 
 	/* Abstract address. */
 	memset(&stream_abstract_addr, 0, sizeof(stream_abstract_addr));
@@ -841,8 +836,9 @@ TEST_F(various_address_sockets, scoped_pathname_sockets)
 		/* Connects with pathname sockets. */
 		stream_pathname_socket = socket(AF_UNIX, SOCK_STREAM, 0);
 		ASSERT_LE(0, stream_pathname_socket);
-		ASSERT_EQ(0, connect(stream_pathname_socket,
-				     &stream_pathname_addr, size_stream));
+		ASSERT_EQ(0,
+			  connect(stream_pathname_socket, &stream_pathname_addr,
+				  sizeof(stream_pathname_addr)));
 		ASSERT_EQ(1, write(stream_pathname_socket, "b", 1));
 		EXPECT_EQ(0, close(stream_pathname_socket));
 
@@ -850,12 +846,13 @@ TEST_F(various_address_sockets, scoped_pathname_sockets)
 		dgram_pathname_socket = socket(AF_UNIX, SOCK_DGRAM, 0);
 		ASSERT_LE(0, dgram_pathname_socket);
 		err = sendto(dgram_pathname_socket, "c", 1, 0,
-			     &dgram_pathname_addr, size_dgram);
+			     &dgram_pathname_addr, sizeof(dgram_pathname_addr));
 		EXPECT_EQ(1, err);
 
 		/* Sends with connection. */
-		ASSERT_EQ(0, connect(dgram_pathname_socket,
-				     &dgram_pathname_addr, size_dgram));
+		ASSERT_EQ(0,
+			  connect(dgram_pathname_socket, &dgram_pathname_addr,
+				  sizeof(dgram_pathname_addr)));
 		ASSERT_EQ(1, write(dgram_pathname_socket, "d", 1));
 		EXPECT_EQ(0, close(dgram_pathname_socket));
 
@@ -910,13 +907,13 @@ TEST_F(various_address_sockets, scoped_pathname_sockets)
 	stream_pathname_socket = socket(AF_UNIX, SOCK_STREAM, 0);
 	ASSERT_LE(0, stream_pathname_socket);
 	ASSERT_EQ(0, bind(stream_pathname_socket, &stream_pathname_addr,
-			  size_stream));
+			  sizeof(stream_pathname_addr)));
 	ASSERT_EQ(0, listen(stream_pathname_socket, backlog));
 
 	dgram_pathname_socket = socket(AF_UNIX, SOCK_DGRAM, 0);
 	ASSERT_LE(0, dgram_pathname_socket);
 	ASSERT_EQ(0, bind(dgram_pathname_socket, &dgram_pathname_addr,
-			  size_dgram));
+			  sizeof(dgram_pathname_addr)));
 
 	/* Sets up abstract servers. */
 	stream_abstract_socket = socket(AF_UNIX, SOCK_STREAM, 0);

base-commit: c07e22414d18559b5c0fd5bc2c2f68f2903f1738
-- 
2.47.3

Re: [PATCH] selftests/landlock: NULL-terminate unix pathname addresses

[Günther Noack]

On Tue, Dec 02, 2025 at 10:51:41PM +0100, Matthieu Buffet wrote:
> The size of Unix pathname addresses is computed in selftests using
> offsetof(struct sockaddr_un, sun_path) + strlen(xxx). It should have
> been that +1, which makes addresses passed to the libc and kernel
> non-NULL-terminated. unix_mkname_bsd() fixes that in Linux so there is
> no harm, but just using sizeof(the address struct) should improve
> readability.
> 
> Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
> ---
> [...]

Thank you very much, this looks good!

Reviewed-by: Günther Noack <gnoack@google.com>

—Günther