Re: [PATCH v4 2/7] landlock: Add UDP connect() access control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] TOCTOU race condition allows bypassing the LANDLOCK_ACCESS_NET_BIND_UDP restriction for autobinding ephemeral ports.
- [High] Spurious access denial when calling connect(AF_UNSPEC) on an unbound UDP socket.
- [Medium] Unmarked data races when reading `inet_sk(sock->sk)->inet_num` and `sock->sk->__sk_common.skc_family`.
--

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

landlock: Add UDP connect() access control

This commit adds support for a second fine-grained UDP access right
LANDLOCK_ACCESS_NET_CONNECT_SEND_UDP. It controls the ability to set the
remote port of a UDP socket via connect(), and includes detection of
when autobind is about to be required.

> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index f9ccb52e7d45..045881f81295 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c

[ ... ]

> @@ -213,6 +216,50 @@ static int current_check_access_socket(struct socket *const sock,
>  	return -EACCES;
>  }
>  
> +static int current_check_autobind_udp_socket(struct socket *const sock)
> +{
> +	struct sockaddr_storage port0 = { 0 };
> +
> +	/*
> +	 * On UDP sockets, if a local port has not already been bound,
> +	 * calling connect() or sending a first datagram has the side
> +	 * effect of autobinding an ephemeral port: we also have to check
> +	 * that the process would have had the right to bind(0) explicitly.
> +	 * Note: socket is not locked, so another thread could do an
> +	 * explicit bind(!=0) on this socket, changing inet_num to non-zero
> +	 * after we read it, but this would only have us enforce an
> +	 * additional bind(0) access check and would not bypass policy.
> +	 */
> +	if (inet_sk(sock->sk)->inet_num != 0)
> +		return 0;

Are unmarked lockless reads of inet_num safe here?

Since the socket is not locked, inet_num can be modified concurrently by
another thread calling bind(), as noted in the comment. Should this read
use READ_ONCE() or data_race() to avoid KCSAN warnings about data races
under the Linux Kernel Memory Model?

This also applies to the lockless read of skc_family below, which can be
concurrently modified by setsockopt(IPV6_ADDRFORM).

Can a TOCTOU race bypass the autobind check?

If a process has an already autobound UDP socket (which lacks
SOCK_BINDPORT_LOCK), what happens if Thread A and Thread B race:

Thread A calls connect(remote_addr). The hook
current_check_autobind_udp_socket() checks inet_num != 0 locklessly,
assumes it is already bound, and skips the BIND_UDP check.

Thread B calls connect(AF_UNSPEC). The Landlock hook explicitly allows this.
The network stack executes udp_disconnect(), which unhashes the socket and
zeroes inet_num.

Thread A proceeds into the network stack (inet_dgram_connect), acquires the
socket lock, observes inet_num == 0, and calls inet_autobind().

Does this allow the socket to successfully bind to a new ephemeral port without
the required BIND_UDP Landlock access check?

> +
> +	/*
> +	 * Construct a struct sockaddr* with port 0 to pretend the
> +	 * process tried to bind() on that address.
> +	 */
> +	port0.ss_family = sock->sk->__sk_common.skc_family;

[ ... ]

> @@ -234,14 +281,22 @@ static int hook_socket_connect(struct socket *const sock,
>  			       const int addrlen)
>  {
>  	access_mask_t access_request;
> +	int ret = 0;
>  
>  	if (sk_is_tcp(sock->sk))
>  		access_request = LANDLOCK_ACCESS_NET_CONNECT_TCP;
> +	else if (sk_is_udp(sock->sk))
> +		access_request = LANDLOCK_ACCESS_NET_CONNECT_SEND_UDP;
>  	else
>  		return 0;
>  
> -	return current_check_access_socket(sock, address, addrlen,
> -					   access_request);
> +	ret = current_check_access_socket(sock, address, addrlen,
> +					  access_request);
> +
> +	if (ret == 0 && sk_is_udp(sock->sk))
> +		ret = current_check_autobind_udp_socket(sock);

Can this unconditionally enforce the bind check for connect(AF_UNSPEC) on
unbound sockets?

If connect(AF_UNSPEC) is called on an unbound UDP socket,
current_check_access_socket() explicitly permits it and returns 0.

However, the hook then unconditionally calls current_check_autobind_udp_socket().
Because the socket is unbound (inet_num == 0), it enforces the BIND_UDP
policy for port 0.

If the process does not have BIND_UDP access, the disconnect operation is denied
with -EACCES. But in the network stack, connect(AF_UNSPEC) merely dissolves
the remote association (udp_disconnect) and never triggers inet_autobind().

Will enforcing a bind check here break legitimate application cleanup behavior?

> +
> +	return ret;
>  }

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