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 <[email protected]>
Cc: Baoyou Xie <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Hideaki YOSHIFUJI <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
net/ipv4/af_inet.c | 27 ++++++++++++++++-----------
net/ipv4/datagram.c | 7 -------
net/ipv4/udp.c | 7 -------
3 files changed, 16 insertions(+), 25 deletions(-)
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);
--
1.8.3.1
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)