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

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

[liujian (CE)]

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?

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);

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

[Jakub Kicinski]

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.

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

[Iwashima, Kuniyuki]

From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 3 Mar 2023 17:06:08 -0800
> 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.

Yes, the same issue happened on 5.15.88, but I forgot to check other stable branches.
I'll check them and post fixes later.
https://lore.kernel.org/stable/20230220133555.140865685@linuxfoundation.org/


> 
> >  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.

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

[liujian (CE)]

> -----Original Message-----
> From: Iwashima, Kuniyuki [mailto:kuniyu@amazon.co.jp]
> Sent: Saturday, March 4, 2023 9:13 AM
> To: liujian (CE) <liujian56@huawei.com>
> Cc: Paolo Abeni <pabeni@redhat.com>; Greg KH
> <gregkh@linuxfoundation.org>; stable@vger.kernel.org; Network
> Development <netdev@vger.kernel.org>; Iwashima, Kuniyuki
> <kuniyu@amazon.co.jp>; Jakub Kicinski <kuba@kernel.org>
> Subject: Re: [Qestion] abort backport commit ("net/ulp: prevent ULP without
> clone op from entering the LISTEN status") in stable-4.19.x
> 
> From: Jakub Kicinski <kuba@kernel.org>
> Date: Fri, 3 Mar 2023 17:06:08 -0800
> > 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.
> 
> Yes, the same issue happened on 5.15.88, but I forgot to check other stable
> branches.
> I'll check them and post fixes later.
> https://lore.kernel.org/stable/20230220133555.140865685@linuxfoundation.
> org/
> 
Thanks Iwashima.
> 
> >
> > >  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.
Got it. Thanks Jakub.