Re: [PATCH v4 3/7] landlock: Add UDP send access control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] TCP Fast Open (MSG_FASTOPEN) via sendmsg()/sendto() bypasses LANDLOCK_ACCESS_NET_CONNECT_TCP enforcement.
- [Medium] Lockless plain reads of concurrently modified socket state (`skc_family` and `inet_num`) introduce data races.
--

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

landlock: Add UDP send access control

This patch adds Landlock access control for sending UDP datagrams to
explicit destinations via sendto() and sendmsg(). It introduces a new
socket_sendmsg hook that applies the LANDLOCK_ACCESS_NET_CONNECT_SEND_UDP
policy.

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

[ ... ]

> @@ -82,6 +84,35 @@ static int current_check_access_socket(struct socket *const sock,
>  			 * inconsistencies and return -EINVAL if needed.
>  			 */
>  			return 0;
> +		} else if (access_request ==
> +			   LANDLOCK_ACCESS_NET_CONNECT_SEND_UDP) {
> +			if (sock->sk->__sk_common.skc_family == AF_INET6) {

The comment below notes that the socket's family can change under our feet
due to concurrent setsockopt(IPV6_ADDRFORM) calls. Since this is a plain C
read of a concurrently modified variable, should we use READ_ONCE() here to
avoid data races and KCSAN warnings?

Additionally, since skc_family is evaluated multiple times in this function,
would it be safer to read it once into a local variable to prevent
inconsistent evaluations due to compiler reloads?

The same question applies to the lockless read of inet_sk(sock->sk)->inet_num
in current_check_autobind_udp_socket(), which also acknowledges concurrent
modifications in its comments.

> +				/*
> +				 * We cannot allow sending UDP datagrams to an
> +				 * explicit AF_UNSPEC address on IPv6 sockets,
> +				 * even if AF_UNSPEC is treated as "no address"
> +				 * on such sockets (so it should always be allowed).
> +				 * That's because the socket's family can change under
> +				 * our feet (if another thread calls setsockopt(IPV6_ADDRFORM))
> +				 * to IPv4, which would then treat AF_UNSPEC as
> +				 * AF_INET.
> +				 */

[ ... ]

> @@ -299,9 +333,33 @@ static int hook_socket_connect(struct socket *const sock,
>  	return ret;
>  }
>  
> +static int hook_socket_sendmsg(struct socket *const sock,
> +			       struct msghdr *const msg, const int size)
> +{
> +	struct sockaddr *const address = msg->msg_name;
> +	const int addrlen = msg->msg_namelen;
> +	access_mask_t access_request;
> +	int ret = 0;
> +
> +	if (sk_is_udp(sock->sk))
> +		access_request = LANDLOCK_ACCESS_NET_CONNECT_SEND_UDP;
> +	else
> +		return 0;

Does returning 0 for all non-UDP sockets leave a pre-existing bypass open for
TCP Fast Open?

If a sandboxed process calls sendmsg() on an unconnected TCP socket with
MSG_FASTOPEN and a target address, it routes through sys_sendmsg() to
security_socket_sendmsg() and then tcp_sendmsg_fastopen(). This entirely
bypasses the connect() wrapper and the security_socket_connect hook where
LANDLOCK_ACCESS_NET_CONNECT_TCP is normally enforced.

Since this patch introduces the socket_sendmsg hook, could we also evaluate
TCP sockets here when address != NULL to enforce the
LANDLOCK_ACCESS_NET_CONNECT_TCP restriction and close this gap?

> +
> +	if (address != NULL)
> +		ret = current_check_access_socket(sock, address, addrlen,
> +						  access_request, false);
> +
> +	if (ret == 0)
> +		ret = current_check_autobind_udp_socket(sock);
> +
> +	return ret;
> +}

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