[PATCH] ipv4: return early for possible invalid uaddr

[PATCH] ipv4: return early for possible invalid uaddr

[Wen Yang]

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

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

Re: [PATCH] ipv4: return early for possible invalid uaddr

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

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

Re: [PATCH] ipv4: return early for possible invalid uaddr

[Wen Yang]

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