netdev - Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added

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] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 2 Jun 2018 14:39:20 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>, edumazet@...gle.com,
        ast@...nel.org, daniel@...earbox.net,
        Dave Watson <davejwatson@...com>
Cc:     netdev@...r.kernel.org
Subject: Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added

On 06/01/2018 12:58 PM, Eric Dumazet wrote:
> 
> 
> On 06/01/2018 03:46 PM, John Fastabend wrote:
>> This fixes a crash where we assign tcp_prot to IPv6 sockets instead
>> of tcpv6_prot.
> 
> ...
> 
>> +	/* ULPs are currently supported only for TCP sockets in ESTABLISHED
>> +	 * state. Supporting sockets in LISTEN state will require us to
>> +	 * modify the accept implementation to clone rather then share the
>> +	 * ulp context.
>> +	 */
>> +	if (sock->sk_state != TCP_ESTABLISHED)
>> +		return -ENOTSUPP;
>> +
>>  	/* 1. If sock map has BPF programs those will be inherited by the
>>  	 * sock being added. If the sock is already attached to BPF programs
>>  	 * this results in an error.
>>
> 
> Next question will be then : What happens if syzbot uses tcp_disconnect() and then listen() ?

Yep we need to fix that as well :( Looks like we can plumb the
unhash callback and remove it from the sockmap when the socket
goes through tcp_disconnect().

This patch should go in as-is though and we can fix the disconnect
issue with a new patch.

Adding Dave Watson to the thread as well because I'm guessing
the disconnect() case is also applicable to TLS. At least I see
a hw handler for unhash but there does not appear to be a handler
in the SW case, at least from a quick glance.

Thanks again!

> 
> Thanks !
>