* [PATCH] selftests/landlock: Remove invalid unix socket bind()
@ 2025-12-01 0:36 Matthieu Buffet
2025-12-02 8:37 ` Günther Noack
0 siblings, 1 reply; 6+ messages in thread
From: Matthieu Buffet @ 2025-12-01 0:36 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, linux-security-module, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] selftests/landlock: Remove invalid unix socket bind() 2025-12-01 0:36 [PATCH] selftests/landlock: Remove invalid unix socket bind() Matthieu Buffet @ 2025-12-02 8:37 ` Günther Noack 2025-12-02 21:46 ` Matthieu Buffet 0 siblings, 1 reply; 6+ messages in thread From: Günther Noack @ 2025-12-02 8:37 UTC (permalink / raw) To: Matthieu Buffet; +Cc: Mickaël Salaün, linux-security-module 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selftests/landlock: Remove invalid unix socket bind() 2025-12-02 8:37 ` Günther Noack @ 2025-12-02 21:46 ` Matthieu Buffet 2025-12-02 21:51 ` [PATCH] selftests/landlock: NULL-terminate unix pathname addresses Matthieu Buffet 0 siblings, 1 reply; 6+ messages in thread From: Matthieu Buffet @ 2025-12-02 21:46 UTC (permalink / raw) To: Günther Noack; +Cc: Mickaël Salaün, linux-security-module 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] selftests/landlock: NULL-terminate unix pathname addresses 2025-12-02 21:46 ` Matthieu Buffet @ 2025-12-02 21:51 ` Matthieu Buffet 2025-12-03 9:48 ` Günther Noack 0 siblings, 1 reply; 6+ messages in thread From: Matthieu Buffet @ 2025-12-02 21:51 UTC (permalink / raw) To: Günther Noack Cc: Mickaël Salaün, linux-security-module, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] selftests/landlock: NULL-terminate unix pathname addresses 2025-12-02 21:51 ` [PATCH] selftests/landlock: NULL-terminate unix pathname addresses Matthieu Buffet @ 2025-12-03 9:48 ` Günther Noack 2025-12-19 20:26 ` Mickaël Salaün 0 siblings, 1 reply; 6+ messages in thread From: Günther Noack @ 2025-12-03 9:48 UTC (permalink / raw) To: Matthieu Buffet; +Cc: Mickaël Salaün, linux-security-module 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selftests/landlock: NULL-terminate unix pathname addresses 2025-12-03 9:48 ` Günther Noack @ 2025-12-19 20:26 ` Mickaël Salaün 0 siblings, 0 replies; 6+ messages in thread From: Mickaël Salaün @ 2025-12-19 20:26 UTC (permalink / raw) To: Günther Noack; +Cc: Matthieu Buffet, linux-security-module Thanks for both patches! Applied to my -next branch. On Wed, Dec 03, 2025 at 10:48:45AM +0100, Günther Noack wrote: > 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-12-19 20:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-12-01 0:36 [PATCH] selftests/landlock: Remove invalid unix socket bind() Matthieu Buffet 2025-12-02 8:37 ` Günther Noack 2025-12-02 21:46 ` Matthieu Buffet 2025-12-02 21:51 ` [PATCH] selftests/landlock: NULL-terminate unix pathname addresses Matthieu Buffet 2025-12-03 9:48 ` Günther Noack 2025-12-19 20:26 ` Mickaël Salaün
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.