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]

Message-ID: <aeff8901-2ca4-99b6-6238-93703946b233@gmail.com>
Date:   Mon, 4 Jun 2018 06:57:28 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        Eric Dumazet <eric.dumazet@...il.com>, edumazet@...gle.com,
        ast@...nel.org, 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/04/2018 06:39 AM, Daniel Borkmann wrote:
> Hey guys,
> 
> On 06/02/2018 11:39 PM, John Fastabend wrote:
>> 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!
> 
> Given the discussion and fixes weren't resolved resp. ready in time for 4.17,
> and last bpf pr for it went out last week, we need to route this via -stable
> once all is hashed out.
> 

OK.

> This fix here therefore needs to be rebased against bpf-next tree, and as far
> as I can see another fix for hash map is also needed to address the same issue.
> 

This fix works for both sockmap and sockhash because they use the same
ulp register and init paths. But, will rebase for net-next and send out
this morning.

> After that, likely also fixes for the disconnect + listen case are needed.
> 

Yep will have a fix today for this.

> (I can use the one here later on for -stable backport, but given merge window
> is open this needs a rebase and a resolution for hash map.)
> 

hash map is also resolved with the same patch but please do queue this
up for -stable.


> Thanks,
> Daniel
>