netdev - Re: [bpf PATCH 2/2] bpf: parse and verdict prog attach may race with bpf map update

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: <ffc1a845-0da3-1588-3c3a-8386cf550362@gmail.com>
Date:   Thu, 17 May 2018 14:06:22 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org
Cc:     netdev@...r.kernel.org
Subject: Re: [bpf PATCH 2/2] bpf: parse and verdict prog attach may race with
 bpf map update

On 05/17/2018 01:31 PM, Daniel Borkmann wrote:
> On 05/16/2018 11:46 PM, John Fastabend wrote:
>> In the sockmap design BPF programs (SK_SKB_STREAM_PARSER and
>> SK_SKB_STREAM_VERDICT) are attached to the sockmap map type and when
>> a sock is added to the map the programs are used by the socket.
>> However, sockmap updates from both userspace and BPF programs can
>> happen concurrently with the attach and detach of these programs.
>>
>> To resolve this we use the bpf_prog_inc_not_zero and a READ_ONCE()
>> primitive to ensure the program pointer is not refeched and
>> possibly NULL'd before the refcnt increment. This happens inside
>> a RCU critical section so although the pointer reference in the map
>> object may be NULL (by a concurrent detach operation) the reference
>> from READ_ONCE will not be free'd until after grace period. This
>> ensures the object returned by READ_ONCE() is valid through the
>> RCU criticl section and safe to use as long as we "know" it may
>> be free'd shortly.
>>
>> Daniel spotted a case in the sock update API where instead of using
>> the READ_ONCE() program reference we used the pointer from the
>> original map, stab->bpf_{verdict|parse}. The problem with this is
>> the logic checks the object returned from the READ_ONCE() is not
>> NULL and then tries to reference the object again but using the
>> above map pointer, which may have already been NULL'd by a parallel
>> detach operation. If this happened bpf_porg_inc_not_zero could
>> dereference a NULL pointer.
>>
>> Fix this by using variable returned by READ_ONCE() that is checked
>> for NULL.
>>
>> Fixes: 2f857d04601a ("bpf: sockmap, remove STRPARSER map_flags and add multi-map support")
>> Reported-by: Daniel Borkmann <daniel@...earbox.net>
>> Signed-off-by: John Fastabend <john.fastabend@...il.com>
>> ---

[...]

> Isn't the same sort of behavior also possible with the bpf_prog_inc_not_zero(stab->bpf_tx_msg)?
> Meaning, we now have verdict and parse covered with the patch, but the original tx_msg we
> fetched earlier via READ_ONCE() where same would apply not (yet)?
> 

Yes, will send a v2 and fix both cases in one shot.