netdev - [bpf PATCH v2 0/6] 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-next>] [day] [month] [year] [list]

Message-ID: <20180614164148.24994.65250.stgit@john-Precision-Tower-5810>
Date:   Thu, 14 Jun 2018 09:44:41 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     ast@...nel.org, daniel@...earbox.net
Cc:     netdev@...r.kernel.org
Subject: [bpf PATCH v2 0/6] BPF fixes for sockhash

This addresses two syzbot issues that lead to identifing (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
error in sockhash.

The first 2 patches address handling IPv4 correctly and then ensuring
that only sockets in ESTABLISHED state can be added. There is then a
follow up fix (patch4) to fix the other issue Eric noted, namely that
we depend on sockets to call tcp_close to remove them from the map.
However, we missed that a socket can transition through
tcp_disconnect() and never call tcp_close() missing our hook. To
resolve this implement the unhash hook which is also called from the
tcp_disconnect() flow.

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. To
fix this we had to restructure the tcp_close() lock handling. This is
done in patch 3.

Finally, during review I noticed the release handler was ommitted
from the upstream code (patch 5) due to an incorrect merge conflict
fix when I ported the code to latest bpf-next before submitting. And
then patch 6 fixes up selftests for the above.

The tcp_disconnect() catch also appears to be missing in kTLS so
a follow up patch will need to address that as well.

v2: Added sock lock to update paths in patch2. Martin noticed this
during review. I was planning to do this in a follow up patch but
I agree its a bit odd to not do it upfront so incorporated into
'bpf: sockmap only allow ESTABLISHED sock state'. In bpf-next we
may consider also taking sock lock on delete/map_free and which
point we could drop some usages of sk_callback_lock but need to
think a bit on the trade-offs of this.

---

John Fastabend (6):
      bpf: sockmap, fix crash when ipv6 sock is added
      bpf: sockmap only allow ESTABLISHED sock state
      bpf: sockhash fix omitted bucket lock in sock_close
      bpf: sockmap, tcp_disconnect to listen transition
      bpf: sockhash, add release routine
      bpf: selftest remove attempts to add LISTEN sockets to sockmap

 0 files changed

--
Signature