| Message ID | 20210811152431.66426-1-wenyang@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] ipv4: fix up inetsw_array coding style | expand |
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. If no, have you tried instead to undo the autobind, if the connect fails ?
在 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. -- Best wishes， Wen
在 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
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 5464818..0f6f138 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1136,23 +1136,23 @@ static int inet_compat_ioctl(struct socket *sock, unsigned int cmd, unsigned lon .prot = &udp_prot, .ops = &inet_dgram_ops, .flags = INET_PROTOSW_PERMANENT, - }, + }, - { + { .type = SOCK_DGRAM, .protocol = IPPROTO_ICMP, .prot = &ping_prot, .ops = &inet_sockraw_ops, .flags = INET_PROTOSW_REUSE, - }, - - { - .type = SOCK_RAW, - .protocol = IPPROTO_IP, /* wild card */ - .prot = &raw_prot, - .ops = &inet_sockraw_ops, - .flags = INET_PROTOSW_REUSE, - } + }, + + { + .type = SOCK_RAW, + .protocol = IPPROTO_IP, /* wild card */ + .prot = &raw_prot, + .ops = &inet_sockraw_ops, + .flags = INET_PROTOSW_REUSE, + } }; #define INETSW_ARRAY_LEN ARRAY_SIZE(inetsw_array)
Switch from a mix of tabs and spaces to just tabs. Signed-off-by: Wen Yang <wenyang@linux.alibaba.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> Cc: David Ahern <dsahern@kernel.org> Cc: Jakub Kicinski <kuba@kernel.org> Cc: netdev@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- net/ipv4/af_inet.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)