netdev - Re: [bpf PATCH v5 0/4] BPF fixes for sockhash

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]

Message-ID: <83c3ab5e-bb9a-066a-385f-1c56f43de967@iogearbox.net>
Date:   Sun, 1 Jul 2018 01:58:24 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     John Fastabend <john.fastabend@...il.com>, ast@...nel.org,
        kafai@...com
Cc:     netdev@...r.kernel.org
Subject: Re: [bpf PATCH v5 0/4] BPF fixes for sockhash

On 06/30/2018 03:17 PM, John Fastabend wrote:
> This addresses two syzbot issues that lead to identifying (by Eric and
> Wei) a class of bugs where we don't correctly check for IPv4/v6
> sockets and their associated state. The second issue was a locking
> omission in sockhash.
> 
> The first patch addresses IPv6 socks and fixing an error where
> sockhash would overwrite the prot pointer with IPv4 prot. To fix
> this build similar solution to TLS ULP. Although we continue to
> allow socks in all states not just ESTABLISH in this patch set
> because as Martin points out there should be no issue with this
> on the sockmap ULP because we don't use the ctx in this code. Once
> multiple ULPs coexist we may need to revisit this. However we
> can do this in *next trees.
> 
> The other issue syzbot found that the tcp_close() handler missed
> locking the hash bucket lock which could result in corrupting the
> sockhash bucket list if delete and close ran at the same time. 
> And also the smap_list_remove() routine was not working correctly
> at all. This was not caught in my testing because in general my
> tests (to date at least lets add some more robust selftest in
> bpf-next) do things in the "expected" order, create map, add socks,
> delete socks, then tear down maps. The tests we have that do the
> ops out of this order where only working on single maps not multi-
> maps so we never saw the issue. Thanks syzbot. The fix is to
> restructure the tcp_close() lock handling. And fix the obvious
> bug in smap_list_remove().
> 
> Finally, during review I noticed the release handler was omitted
> from the upstream code (patch 4) due to an incorrect merge conflict
> fix when I ported the code to latest bpf-next before submitting.
> This would leave references to the map around if the user never
> closes the map.
> 
> v3: rework patches, dropping ESTABLISH check and adding rcu
>     annotation along with the smap_list_remove fix
> 
> v4: missed one more case where maps was being accessed without
>     the sk_callback_lock, spoted by Martin as well.
> 
> v5: changed to use a specific lock for maps and reduced callback
>     lock so that it is only used to gaurd sk callbacks. I think
>     this makes the logic a bit cleaner and avoids confusion
>     ovoer what each lock is doing.
> 
> Also big thanks to Martin for thorough review he caught at least
> one case where I missed a rcu_call().

Applied it to bpf, thanks John!