Re: [RFC PATCH v2 3/8] landlock: Fix inconsistency of errors for TCP actions

[Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>]

On 10/17/2024 2:04 PM, Mikhail Ivanov wrote:

[...]

> +static int
> +check_tcp_connect_consistency_and_get_port(struct socket *const sock,
> +					   struct sockaddr *const address,
> +					   const int addrlen, __be16 *port)
> +{
> +	int err = 0;
> +	struct sock *const sk = sock->sk;
> +
> +	/* Cf. __inet_stream_connect(). */
> +	lock_sock(sk);
> +	switch (sock->state) {
> +	default:
> +		err = -EINVAL;
> +		break;
> +	case SS_CONNECTED:
> +		err = -EISCONN;
> +		break;
> +	case SS_CONNECTING:
> +		/*
> +		 * Calling connect(2) on nonblocking socket with SYN_SENT or SYN_RECV
> +		 * state immediately returns -EISCONN and -EALREADY (Cf. __inet_stream_connect()).
> +		 *
> +		 * This check is not tested with kselftests.
> +		 */
> +		if ((sock->file->f_flags & O_NONBLOCK) &&
> +		    ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))) {
> +			if (inet_test_bit(DEFER_CONNECT, sk))
> +				err = -EISCONN;
> +			else
> +				err = -EALREADY;
> +			break;
> +		}
> +
> +		/*
> +		 * Current state is possible in two cases:
> +		 * 1. connect(2) is called upon nonblocking socket and previous
> +		 *    connection attempt was closed by RST packet (therefore socket is
> +		 *    in TCP_CLOSE state). In this case connect(2) calls
> +		 *    sk_prot->disconnect(), changes socket state and increases number
> +		 *    of disconnects.
> +		 * 2. connect(2) is called twice upon socket with TCP_FASTOPEN_CONNECT
> +		 *    option set. If socket state is TCP_CLOSE connect(2) does the
> +		 *    same logic as in point 1 case. Otherwise connect(2) may freeze
> +		 *    after inet_wait_for_connect() call since SYN was never sent.
> +		 *
> +		 * For both this cases Landlock cannot provide error consistency since
> +		 * 1. Both cases involve executing some network stack logic and changing
> +		 *    the socket state.
> +		 * 2. It cannot omit access check and allow network stack handle error
> +		 *    consistency since socket can change its state to SS_UNCONNECTED
> +		 *    before it will be locked again in inet_stream_connect().
> +		 *
> +		 * Therefore it is only possible to return 0 and check access right with
> +		 * check_access_port() helper.
> +		 */
> +		release_sock(sk);
> +		return 0;

Returning 0 is incorrect since port was not extracted yet. Last two
lines should be replaced with a "break" to let further switch safely
extract a port.

This also requires fix in tcp_errors_consistency.connect kselftest.

> +	case SS_UNCONNECTED:
> +		if (sk->sk_state != TCP_CLOSE)
> +			err = -EISCONN;
> +		break;
> +	}
> +	release_sock(sk);
> +
> +	if (err)
> +		return err;
> +
> +	/* IPV6_ADDRFORM can change sk->sk_family under us. */
> +	switch (READ_ONCE(sk->sk_family)) {
> +	case AF_INET:
> +		/* Cf. tcp_v4_connect(). */
> +		if (addrlen < sizeof(struct sockaddr_in))
> +			return -EINVAL;
> +		if (address->sa_family != AF_INET)
> +			return -EAFNOSUPPORT;
> +
> +		*port = ((struct sockaddr_in *)address)->sin_port;
> +		break;
> +#if IS_ENABLED(CONFIG_IPV6)
> +	case AF_INET6:
> +		/* Cf. tcp_v6_connect(). */
> +		if (addrlen < SIN6_LEN_RFC2133)
> +			return -EINVAL;
> +		if (address->sa_family != AF_INET6)
> +			return -EAFNOSUPPORT;
> +
> +		*port = ((struct sockaddr_in6 *)address)->sin6_port;
> +		break;
> +#endif /* IS_ENABLED(CONFIG_IPV6) */
> +	default:
> +		WARN_ON_ONCE(0);
> +		return -EACCES;
> +	}
> +
> +	return 0;
> +}