netdev - Re: [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type

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:   Fri, 19 Oct 2018 16:38:26 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org,
        eric.dumazet@...il.com
Cc:     netdev@...r.kernel.org
Subject: Re: [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type

On 10/19/2018 03:57 PM, Daniel Borkmann wrote:
> On 10/20/2018 12:51 AM, Daniel Borkmann wrote:
>> On 10/18/2018 10:58 PM, John Fastabend wrote:
>>> We check if the sk_user_data (the psock in skmsg) is in fact a sockmap
>>> type to late, after we read the refcnt which is an error. This
>>> series moves the check up before reading refcnt and also adds a test
>>> to test_maps to test trying to add a KCM socket into a sockmap.
>>>
>>> While reviewig this code I also found an issue with KCM and kTLS
>>> where each uses sk_data_ready hooks and associated stream parser
>>> breaking expectations in kcm, ktls or both. But that fix will need
>>> to go to net.
>>>
>>> Thanks to Eric for reporting.
>>>
>>> v2: Fix up file +/- my scripts lost track of them
>>> v3: return EBUSY if refcnt is zero
>>>
>>> John Fastabend (2):
>>>   bpf: skmsg, fix psock create on existing kcm/tls port
>>>   bpf: test_maps add a test to catch kcm + sockmap
>>>
>>>  include/linux/skmsg.h                     | 25 +++++++++---
>>>  net/core/sock_map.c                       | 11 +++---
>>>  tools/testing/selftests/bpf/Makefile      |  2 +-
>>>  tools/testing/selftests/bpf/sockmap_kcm.c | 14 +++++++
>>>  tools/testing/selftests/bpf/test_maps.c   | 64 ++++++++++++++++++++++++++++++-
>>>  5 files changed, 103 insertions(+), 13 deletions(-)
>>>  create mode 100644 tools/testing/selftests/bpf/sockmap_kcm.c
>>
>> Applied, thanks!
> 
> Fyi, I've only applied patch 1/2 for now to get the bug fixed. The patch 2/2 throws
> a bunch of warnings that look like the below. Also, I think we leak kcm socket in
> error paths and once we're done with testing, so would be good to close it once
> unneeded. Please respin the test as a stand-alone commit, thanks:
> 

Thanks, I didn't see the warnings below locally but will look
into spinning a good version tonight with the closing sock fix
as well.

John

> [...]
> bpf-next/tools/testing/selftests/bpf/libbpf.a -lcap -lelf -lrt -lpthread -o /home/darkstar/trees/bpf-next-ok/tools/testing/selftests/bpf/test_maps
> test_maps.c: In function ‘test_sockmap’:
> test_maps.c:869:0: warning: "AF_KCM" redefined
>  #define AF_KCM 41
> 
> In file included from /usr/include/sys/socket.h:38:0,
>                  from test_maps.c:21:
> /usr/include/bits/socket.h:133:0: note: this is the location of the previous definition
>  #define AF_KCM  PF_KCM
>