[Qestion] abort backport commit ("net/ulp: prevent ULP without clone op from entering the LISTEN status") in stable-4.19.x

Commit Message

liujian (CE) March 3, 2023, 10:52 a.m. UTC

Hi, 

When I was working on CVE-2023-0461, I found the below backport commit in stable-4.19.x maybe something wrong?

755193f2523c ("net/ulp: prevent ULP without clone op from entering the LISTEN status") 

1.  err = -EADDRINUSE in inet_csk_listen_start() was removed. But it is the error code when get_port() fails. 
 2. The change in __tcp_set_ulp() should not be discarded?

Can I modify the patch like below?

Comments

Jakub Kicinski March 4, 2023, 1:06 a.m. UTC | #1

On Fri, 3 Mar 2023 10:52:15 +0000 liujian (CE) wrote:
> When I was working on CVE-2023-0461, I found the below backport commit in stable-4.19.x maybe something wrong?
> 
> 755193f2523c ("net/ulp: prevent ULP without clone op from entering the LISTEN status") 
> 
> 1.  err = -EADDRINUSE in inet_csk_listen_start() was removed. But it
>     is the error code when get_port() fails. 

I think you're right, we should add setting the err back.

>  2. The change in __tcp_set_ulp() should not be discarded?

That part should be fine, all ULPs in 4.19 (i.e. TLS) should fail
the ->init() call if sk_state != ESTABLISHED.

Patch

diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 0a69f92da71b..3ed2f753628e 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -903,11 +903,25 @@  void inet_csk_prepare_forced_close(struct sock *sk)
 }
 EXPORT_SYMBOL(inet_csk_prepare_forced_close);
 
+static int inet_ulp_can_listen(const struct sock *sk)
+{
+       const struct inet_connection_sock *icsk = inet_csk(sk);
+
+       if (icsk->icsk_ulp_ops)
+               return -EINVAL;
+
+       return 0;
+}
+
 int inet_csk_listen_start(struct sock *sk, int backlog)
 {
        struct inet_connection_sock *icsk = inet_csk(sk);
        struct inet_sock *inet = inet_sk(sk);
-       int err = -EADDRINUSE;
+       int err;
+
+       err = inet_ulp_can_listen(sk);
+       if (unlikely(err))
+               return err;
 
        reqsk_queue_alloc(&icsk->icsk_accept_queue);
 
@@ -921,6 +935,7 @@  int inet_csk_listen_start(struct sock *sk, int backlog)
         * after validation is complete.
         */
        inet_sk_state_store(sk, TCP_LISTEN);
+       err = -EADDRINUSE;
        if (!sk->sk_prot->get_port(sk, inet->inet_num)) {
                inet->inet_sport = htons(inet->inet_num);
 
diff --git a/net/ipv4/tcp_ulp.c b/net/ipv4/tcp_ulp.c
index a5995bb2eaca..437987be68be 100644
--- a/net/ipv4/tcp_ulp.c
+++ b/net/ipv4/tcp_ulp.c
@@ -152,6 +152,11 @@  int tcp_set_ulp(struct sock *sk, const char *name)
                return -ENOENT;
        }
 
+       if (sk->sk_state == TCP_LISTEN) {
+               module_put(ulp_ops->owner);
+               return -EINVAL
+       }
+
        err = ulp_ops->init(sk);
        if (err) {
                module_put(ulp_ops->owner);