Re: [PATCH v4 5/7] selftests/landlock: Add tests for sendmsg()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Test Hang Vulnerability due to EXPECT_EQ before blocking socket reads
- [Medium] Stale Data Validation due to missing audit_count_records() call
- [Low] File descriptor and resource leaks in test paths
--

commit a3c5aefc4ced3649cf0e1efaac53d8674cff7d76
Author: Matthieu Buffet <matthieu@buffet.re>

selftests/landlock: Add tests for sendmsg()

This commit adds tests specific to UDP sendmsg() in the protocol_* variants
to ensure behaviour is consistent across AF_INET, AF_INET6 and AF_UNIX.

> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c

[ ... ]

> @@ -1119,6 +1274,441 @@ TEST_F(protocol, connect_unspec)
> +TEST_F(protocol, sendmsg_stream)
> +{

[ ... ]

> +	/* Send to a truncated (invalid) address. */
> +	res = sendto_variant_addrlen(client_fd, &self->srv0,
> +				     get_addrlen(&self->srv0, true) - 1, "D", 1,
> +				     0);
> +	if (variant->prot.domain == AF_UNIX) {
> +		EXPECT_EQ(-EISCONN, res);
> +	} else {
> +		EXPECT_EQ(0, res);
> +		EXPECT_EQ(1, recv(srv0_fd, read_buf, 1, 0))

If the send operation fails, EXPECT_EQ logs the failure but allows the test
to continue. Will the subsequent recv() block indefinitely since no data was
sent, causing the test suite to hang until a timeout occurs?

Should this be ASSERT_EQ instead?

[ ... ]

> +TEST_F(protocol, sendmsg_unspec)
> +{

[ ... ]

> +	res = sendto_variant(client_fd, &self->unspec_srv0, "B", 1, 0);
> +	if (variant->prot.domain == AF_INET6) {

[ ... ]

> +	} else if (variant->prot.domain == AF_INET) {
> +		EXPECT_EQ(0, res);
> +		EXPECT_EQ(1, read(srv0_fd, read_buf, 1))

Could this read() also block indefinitely if the sendto_variant() above fails,
similar to the recv() in sendmsg_stream?

[ ... ]

> @@ -2342,4 +2938,58 @@ TEST_F(audit, connect)
> +TEST_F(audit, sendmsg)
> +{
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_net = LANDLOCK_ACCESS_NET_CONNECT_SEND_UDP |
> +				      LANDLOCK_ACCESS_NET_BIND_UDP,
> +	};
> +	const struct landlock_net_port_attr rule = {
> +		.allowed_access = LANDLOCK_ACCESS_NET_CONNECT_SEND_UDP,
> +		.port = self->srv1.port,
> +	};
> +	const int ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	struct audit_records records;
> +	int sock_fd;
> +
> +	/* Sendmsg on stream sockets is never denied. */
> +	if (variant->prot.type != SOCK_DGRAM)
> +		return;

Does this early return leak the ruleset_fd created just above it?

Could the check for SOCK_DGRAM be moved before creating the ruleset?

[ ... ]

> +	EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
> +	EXPECT_EQ(0, records.access);
> +	EXPECT_EQ(1, records.domain);
> +
> +	/* Check that autobind generates a denied bind event. */
> +	EXPECT_EQ(-EACCES, sendto_variant(sock_fd, &self->srv1, "A", 1, 0));
> +	EXPECT_EQ(0, matches_auditlog(self->audit_fd, "net\\.bind_udp", NULL,
> +				      NULL, NULL));
> +	EXPECT_EQ(0, records.access);
> +	EXPECT_EQ(1, records.domain);

Are we checking stale data here? It looks like a call to
audit_count_records(self->audit_fd, &records) is missing before checking
records.access and records.domain for this second event.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260502124306.3975990-1-matthieu@buffet.re?part=5