Re: [PATCH v2 2/2] net: return early for possible invalid uaddr

[Wen Yang]

在 2021/8/13 上午1:35, Wen Yang 写道:
> 在 2021/8/12 上午12:11, Eric Dumazet 写道:
> > On 8/11/21 5:24 PM, Wen Yang wrote:
> > > The inet_dgram_connect() first calls inet_autobind() to select an
> > > ephemeral port, then checks uaddr in udp_pre_connect() or
> > > __ip4_datagram_connect(), but the port is not released until the socket
> > > is closed. This could cause performance issues or even exhaust ephemeral
> > > ports if a malicious user makes a large number of UDP connections with
> > > invalid uaddr and/or addr_len.
> >
> > This is a big patch.
> >
> > Can the malicious user still use a large number of UDP sockets,
> > with valid uaddr/add_len and consequently exhaust ephemeral ports ?
> >
> > If yes, it does not seem your patch is helping.
>
> Thank you for your comments.
> However, we could make these optimizations:
>
> 1, If the user passed in some invalid parameters, we should return as
> soon as possible. We shouldn't assume that these parameters are valid
> first, then do some real work (such as select an ephemeral port), and
> then finally check that they are indeed valid or not.
>
> 2. Unify the code for checking parameters in udp_pre_connect() and
> __ip4_datagram_connect() to make the code clearer.
>
> > If no, have you tried instead to undo the autobind, if the connect 
> > fails ?
>
> Thanks. Undo the autobind is useful if the connect fails.
> We will add this logic and submit the v3 patch later.

Hello, there is no undo autobind for udp. If this logic is added, the 
patch will be bigger; maybe we can release this ephemeral port through 
unhash()?

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 8a8dba7..43947d8 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -563,6 +563,7 @@ int inet_dgram_connect(struct socket *sock, struct 
sockaddr *uaddr,
                       int addr_len, int flags)
 {
        struct sock *sk = sock->sk;
+       bool autobind;
        int err;

        if (addr_len < sizeof(uaddr->sa_family))
@@ -581,9 +582,17 @@ int inet_dgram_connect(struct socket *sock, struct 
sockaddr *uaddr,
                        return err;
        }

-       if (data_race(!inet_sk(sk)->inet_num) && inet_autobind(sk))
+       autobind = data_race(!inet_sk(sk)->inet_num);
+       if (autobind && inet_autobind(sk))
                return -EAGAIN;
-       return sk->sk_prot->connect(sk, uaddr, addr_len);
+
+       err = sk->sk_prot->connect(sk, uaddr, addr_len);
+       if (err && autobind) {
+               if (sk->sk_prot->unhash)
+                       sk->sk_prot->unhash(sk);
+       }
+
+       return err;
 }
 EXPORT_SYMBOL(inet_dgram_connect);

Could you kindly give some suggestions?

In addition, the previous v2 patch detects errors before bind and 
returns earlier, which should be reasonable.


--
Best wishes，
Wen