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;
+}
