Re: [RFC PATCH v1 3/3] landlock: Fix TCP handling of short AF_UNSPEC addresses

["Mickaël Salaün" <mic@digikod.net>]

On Mon, Oct 27, 2025 at 08:07:26PM +0100, Matthieu Buffet wrote:
> current_check_access_socket() treats AF_UNSPEC addresses as
> AF_INET ones, and only later adds special case handling to
> allow connect(AF_UNSPEC), and on IPv4 sockets
> bind(AF_UNSPEC+INADDR_ANY).
> This would be fine except AF_UNSPEC addresses can be as
> short as a bare AF_UNSPEC sa_family_t field, and nothing
> more. The AF_INET code path incorrectly enforces a length of
> sizeof(struct sockaddr_in) instead.

Good catch!

> 
> Move AF_UNSPEC edge case handling up inside the switch-case,
> before the address is (potentially incorrectly) treated as
> AF_INET.

And that's cleaner this way too.

> 
> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
> Signed-off-by: Matthieu Buffet <matthieu@buffet.re>

I pushed this series to my -next branch, but moving the tests after the
kernel fix. This makes it possible to cleanly bisect commits.  Thanks!

> ---
>  security/landlock/net.c | 118 +++++++++++++++++++++++-----------------
>  1 file changed, 67 insertions(+), 51 deletions(-)
> 
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index 1f3915a90a80..e6367e30e5b0 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -71,6 +71,61 @@ static int current_check_access_socket(struct socket *const sock,
>  
>  	switch (address->sa_family) {
>  	case AF_UNSPEC:
> +		if (access_request == LANDLOCK_ACCESS_NET_CONNECT_TCP) {
> +			/*
> +			 * Connecting to an address with AF_UNSPEC dissolves
> +			 * the TCP association, which have the same effect as
> +			 * closing the connection while retaining the socket
> +			 * object (i.e., the file descriptor).  As for dropping
> +			 * privileges, closing connections is always allowed.
> +			 *
> +			 * For a TCP access control system, this request is
> +			 * legitimate. Let the network stack handle potential
> +			 * inconsistencies and return -EINVAL if needed.
> +			 */
> +			return 0;
> +		} else if (access_request == LANDLOCK_ACCESS_NET_BIND_TCP) {
> +			/*
> +			 * Binding to an AF_UNSPEC address is treated
> +			 * differently by IPv4 and IPv6 sockets. The socket's
> +			 * family may change under our feet due to
> +			 * setsockopt(IPV6_ADDRFORM), but that's ok: we either
> +			 * reject entirely or require
> +			 * %LANDLOCK_ACCESS_NET_BIND_TCP for the given port, so
> +			 * it cannot be used to bypass the policy.
> +			 *
> +			 * IPv4 sockets map AF_UNSPEC to AF_INET for
> +			 * retrocompatibility for bind accesses, only if the
> +			 * address is INADDR_ANY (cf. __inet_bind). IPv6
> +			 * sockets always reject it.
> +			 *
> +			 * Checking the address is required to not wrongfully
> +			 * return -EACCES instead of -EAFNOSUPPORT or -EINVAL.
> +			 * We could return 0 and let the network stack handle
> +			 * these checks, but it is safer to return a proper
> +			 * error and test consistency thanks to kselftest.
> +			 */
> +			if (sock->sk->__sk_common.skc_family == AF_INET) {
> +				const struct sockaddr_in *const sockaddr =
> +					(struct sockaddr_in *)address;
> +
> +				if (addrlen < sizeof(struct sockaddr_in))
> +					return -EINVAL;
> +
> +				if (sockaddr->sin_addr.s_addr !=
> +				    htonl(INADDR_ANY))
> +					return -EAFNOSUPPORT;
> +			} else {
> +				if (addrlen < SIN6_LEN_RFC2133)
> +					return -EINVAL;
> +				else
> +					return -EAFNOSUPPORT;
> +			}
> +		} else {
> +			WARN_ON_ONCE(1);
> +		}
> +		/* Only for bind(AF_UNSPEC+INADDR_ANY) on IPv4 socket. */
> +		fallthrough;
>  	case AF_INET: {
>  		const struct sockaddr_in *addr4;
>  
> @@ -119,57 +174,18 @@ static int current_check_access_socket(struct socket *const sock,
>  		return 0;
>  	}
>  
> -	/* Specific AF_UNSPEC handling. */
> -	if (address->sa_family == AF_UNSPEC) {
> -		/*
> -		 * Connecting to an address with AF_UNSPEC dissolves the TCP
> -		 * association, which have the same effect as closing the
> -		 * connection while retaining the socket object (i.e., the file
> -		 * descriptor).  As for dropping privileges, closing
> -		 * connections is always allowed.
> -		 *
> -		 * For a TCP access control system, this request is legitimate.
> -		 * Let the network stack handle potential inconsistencies and
> -		 * return -EINVAL if needed.
> -		 */
> -		if (access_request == LANDLOCK_ACCESS_NET_CONNECT_TCP)
> -			return 0;
> -
> -		/*
> -		 * For compatibility reason, accept AF_UNSPEC for bind
> -		 * accesses (mapped to AF_INET) only if the address is
> -		 * INADDR_ANY (cf. __inet_bind).  Checking the address is
> -		 * required to not wrongfully return -EACCES instead of
> -		 * -EAFNOSUPPORT.
> -		 *
> -		 * We could return 0 and let the network stack handle these
> -		 * checks, but it is safer to return a proper error and test
> -		 * consistency thanks to kselftest.
> -		 */
> -		if (access_request == LANDLOCK_ACCESS_NET_BIND_TCP) {
> -			/* addrlen has already been checked for AF_UNSPEC. */
> -			const struct sockaddr_in *const sockaddr =
> -				(struct sockaddr_in *)address;
> -
> -			if (sock->sk->__sk_common.skc_family != AF_INET)
> -				return -EINVAL;
> -
> -			if (sockaddr->sin_addr.s_addr != htonl(INADDR_ANY))
> -				return -EAFNOSUPPORT;
> -		}
> -	} else {
> -		/*
> -		 * Checks sa_family consistency to not wrongfully return
> -		 * -EACCES instead of -EINVAL.  Valid sa_family changes are
> -		 * only (from AF_INET or AF_INET6) to AF_UNSPEC.
> -		 *
> -		 * We could return 0 and let the network stack handle this
> -		 * check, but it is safer to return a proper error and test
> -		 * consistency thanks to kselftest.
> -		 */
> -		if (address->sa_family != sock->sk->__sk_common.skc_family)
> -			return -EINVAL;
> -	}
> +	/*
> +	 * Checks sa_family consistency to not wrongfully return
> +	 * -EACCES instead of -EINVAL.  Valid sa_family changes are
> +	 * only (from AF_INET or AF_INET6) to AF_UNSPEC.
> +	 *
> +	 * We could return 0 and let the network stack handle this
> +	 * check, but it is safer to return a proper error and test
> +	 * consistency thanks to kselftest.
> +	 */
> +	if (address->sa_family != sock->sk->__sk_common.skc_family &&
> +	    address->sa_family != AF_UNSPEC)
> +		return -EINVAL;
>  
>  	id.key.data = (__force uintptr_t)port;
>  	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
> -- 
> 2.47.2
> 
>