| Message ID | 20210807171938.38501-1-wenyang@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | ipv4: return early for possible invalid uaddr | expand |
On Sun, 8 Aug 2021 01:19:38 +0800 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. > > We should return early for invalid uaddr to improve performance and > simplify the code a bit, The performance improvement would be if the benchmark is calling connect with invalid arguments? That seems like an extremely rare scenario in real life. > and also switch from a mix of tabs and spaces to just tabs. Please never mix unrelated whitespace cleanup into patches making real code changes. > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c > index 5464818..97b6fc4 100644 > --- a/net/ipv4/af_inet.c > +++ b/net/ipv4/af_inet.c > @@ -569,6 +569,11 @@ int inet_dgram_connect(struct socket *sock, struct sockaddr *uaddr, > if (uaddr->sa_family == AF_UNSPEC) > return sk->sk_prot->disconnect(sk, flags); > > + if (uaddr->sa_family != AF_INET) > + return -EAFNOSUPPORT; And what about IPv6 which also calls this function? > + if (addr_len < sizeof(struct sockaddr_in)) > + return -EINVAL; > + > if (BPF_CGROUP_PRE_CONNECT_ENABLED(sk)) { > err = sk->sk_prot->pre_connect(sk, uaddr, addr_len); > if (err)
在 2021/8/10 上午6:32, Jakub Kicinski 写道: > On Sun, 8 Aug 2021 01:19:38 +0800 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. >> >> We should return early for invalid uaddr to improve performance and >> simplify the code a bit, > > The performance improvement would be if the benchmark is calling > connect with invalid arguments? That seems like an extremely rare > scenario in real life. > Thanks for your comments. On the one hand, it is the performance impact, but we also found that it may cause DoS: simulate a scenario where udp connect is frequently performed (illegal addrlen, and the socket is not closed), the local ports will be exhausted quickly. >> and also switch from a mix of tabs and spaces to just tabs. > > Please never mix unrelated whitespace cleanup into patches making real > code changes. > OK. >> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c >> index 5464818..97b6fc4 100644 >> --- a/net/ipv4/af_inet.c >> +++ b/net/ipv4/af_inet.c >> @@ -569,6 +569,11 @@ int inet_dgram_connect(struct socket *sock, struct sockaddr *uaddr, >> if (uaddr->sa_family == AF_UNSPEC) >> return sk->sk_prot->disconnect(sk, flags); >> >> + if (uaddr->sa_family != AF_INET) >> + return -EAFNOSUPPORT; > > And what about IPv6 which also calls this function? > Sorry that currently only ipv4 has been modified, we will continue to improve, and the v2 patch will be submitted later, thank you. -- Best wishes， Wen >> + if (addr_len < sizeof(struct sockaddr_in)) >> + return -EINVAL; >> + >> if (BPF_CGROUP_PRE_CONNECT_ENABLED(sk)) { >> err = sk->sk_prot->pre_connect(sk, uaddr, addr_len); >> if (err)
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 5464818..97b6fc4 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -569,6 +569,11 @@ int inet_dgram_connect(struct socket *sock, struct sockaddr *uaddr, if (uaddr->sa_family == AF_UNSPEC) return sk->sk_prot->disconnect(sk, flags); + if (uaddr->sa_family != AF_INET) + return -EAFNOSUPPORT; + if (addr_len < sizeof(struct sockaddr_in)) + return -EINVAL; + if (BPF_CGROUP_PRE_CONNECT_ENABLED(sk)) { err = sk->sk_prot->pre_connect(sk, uaddr, addr_len); if (err) @@ -1136,23 +1141,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) diff --git a/net/ipv4/datagram.c b/net/ipv4/datagram.c index 4a8550c..81aae1d 100644 --- a/net/ipv4/datagram.c +++ b/net/ipv4/datagram.c @@ -27,13 +27,6 @@ int __ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len int oif; int err; - - if (addr_len < sizeof(*usin)) - return -EINVAL; - - if (usin->sin_family != AF_INET) - return -EAFNOSUPPORT; - sk_dst_reset(sk); oif = sk->sk_bound_dev_if; diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index 62cd4cd..1ef0770 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -1928,13 +1928,6 @@ int udp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock, int udp_pre_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len) { - /* This check is replicated from __ip4_datagram_connect() and - * intended to prevent BPF program called below from accessing bytes - * that are out of the bound specified by user in addr_len. - */ - if (addr_len < sizeof(struct sockaddr_in)) - return -EINVAL; - return BPF_CGROUP_RUN_PROG_INET4_CONNECT_LOCK(sk, uaddr); } EXPORT_SYMBOL(udp_pre_connect);
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. We should return early for invalid uaddr to improve performance and simplify the code a bit, and also switch from a mix of tabs and spaces to just tabs. Signed-off-by: Wen Yang <wenyang@linux.alibaba.com> Cc: Baoyou Xie <baoyou.xie@alibaba-inc.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 | 27 ++++++++++++++++----------- net/ipv4/datagram.c | 7 ------- net/ipv4/udp.c | 7 ------- 3 files changed, 16 insertions(+), 25 deletions(-)