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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 11 Aug 2021 11:41:15 +0800
From:   Wen Yang <wenyang@...ux.alibaba.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, David Ahern <dsahern@...nel.org>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Baoyou Xie <baoyou.xie@...baba-inc.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ipv4: return early for possible invalid uaddr



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