[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a68a9e2c-fb2c-3cd7-c076-ecf95e94606d@huawei-partners.com>
Date: Thu, 17 Oct 2024 14:34:59 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: <mic@...ikod.net>, <gnoack@...gle.com>
CC: <willemdebruijn.kernel@...il.com>, <matthieu@...fet.re>,
<linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
<netfilter-devel@...r.kernel.org>, <yusongping@...wei.com>,
<artem.kuzin@...wei.com>, <konstantin.meskhidze@...wei.com>
Subject: Re: [RFC PATCH v2 3/8] landlock: Fix inconsistency of errors for TCP
actions
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;
> +}
Powered by blists - more mailing lists