Re: [PATCH v4 4/7] selftests/landlock: Add UDP bind/connect tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The `SO_RCVTIMEO` and `SO_SNDTIMEO` socket timeouts added in `socket_variant()` are set to an aggressively short 100 milliseconds.
- [Medium] The patch introduces a file descriptor leak in `TEST_F(port_specific, bind_connect_1023)` by overwriting `connect_fd` without closing the previously allocated socket.
- [Low] The 64-bit Landlock access rights bitmasks are assigned to signed 32-bit `const int` variables, which discards the upper 32 bits.
--

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

selftests/landlock: Add UDP bind/connect tests
    
Make basic changes to the existing bind() and connect() test suite to
cover UDP restriction.

> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index 016c7277e370..568a6ed7139c 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c

[ ... ]

>  static int socket_variant(const struct service_fixture *const srv)
>  {
> +	/* Arbitrary value just to not block other tests indefinitely. */
> +	const struct timeval timeout = {
> +		.tv_sec = 0,
> +		.tv_usec = 100000,
> +	};

Could this 100 millisecond timeout cause false-positive test failures in heavily
loaded CI environments or nested VMs?

In test_bind_and_connect(), the parent process calls accept() or read() and
waits for an unsynchronized forked child process to execute socket(), connect(),
and write(). If process scheduling delays the child, the parent's blocking
operations might time out and return -EAGAIN.

[ ... ]

> @@ -737,16 +895,20 @@ TEST_F(protocol, connect)
>  
>  TEST_F(protocol, bind_unspec)
>  {
> +	const int bind_access = (variant->sandbox == TCP_SANDBOX ?
> +					 LANDLOCK_ACCESS_NET_BIND_TCP :
> +					 LANDLOCK_ACCESS_NET_BIND_UDP);

Should bind_access use __u64 instead of int?

The 64-bit Landlock access rights bitmasks are assigned to signed 32-bit int
variables here and in several other tests, which discards the upper 32 bits.
While current flags use only the lower bits, this type mismatch might risk
future sign-extension or truncation bugs if Landlock introduces flags using
higher bits.

[ ... ]

> @@ -1815,23 +2035,35 @@ TEST_F(port_specific, bind_connect_1023)
>  	int bind_fd, connect_fd, ret;
>  
>  	/* Adds a rule layer with bind and connect actions. */
> -	if (variant->sandbox == TCP_SANDBOX) {
> +	if (variant->sandbox == TCP_SANDBOX ||
> +	    variant->sandbox == UDP_SANDBOX) {
> +		const int bind_right = (variant->sandbox == TCP_SANDBOX ?
> +						LANDLOCK_ACCESS_NET_BIND_TCP :
> +						LANDLOCK_ACCESS_NET_BIND_UDP);
> +		const int access_rights =
> +			(variant->sandbox == TCP_SANDBOX ?
> +				 (LANDLOCK_ACCESS_NET_BIND_TCP |
> +				  LANDLOCK_ACCESS_NET_CONNECT_TCP) :
> +				 (LANDLOCK_ACCESS_NET_BIND_UDP |
> +				  LANDLOCK_ACCESS_NET_CONNECT_SEND_UDP));

Similar to the above, should bind_right and access_rights be defined as __u64
to match struct landlock_ruleset_attr.handled_access_net?

[ ... ]

> @@ -1867,8 +2104,19 @@ TEST_F(port_specific, bind_connect_1023)
>  	ret = bind_variant(bind_fd, &self->srv0);
>  	clear_cap(_metadata, CAP_NET_BIND_SERVICE);
>  	EXPECT_EQ(0, ret);
> -	EXPECT_EQ(0, listen(bind_fd, backlog));
> +	if (variant->prot.type == SOCK_STREAM)
> +		EXPECT_EQ(0, listen(bind_fd, backlog));
>  
> +	connect_fd = socket_variant(&self->srv0);
> +	ASSERT_LE(0, connect_fd);

Does this overwrite the connect_fd variable and leak the previously allocated
socket file descriptor without calling close()?

connect_fd appears to be allocated via socket_variant() earlier at the start
of TEST_F(port_specific, bind_connect_1023).

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