netdev - Re: [PATCH bpf-next v6 1/4] bpf: sockmap, refactor sockmap routines to work with hashmap

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: <daf91cc6-a8cc-89aa-1450-9a10ea9a0728@gmail.com>
Date:   Wed, 16 May 2018 14:46:33 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH bpf-next v6 1/4] bpf: sockmap, refactor sockmap routines
 to work with hashmap

On 05/15/2018 12:19 PM, Daniel Borkmann wrote:
> On 05/14/2018 07:00 PM, John Fastabend wrote:
> [...]


[...]

> 
> As you say in the comment above the function wrt locking notes that the
> __sock_map_ctx_update_elem() can be called concurrently.
> 
>   All operations operate on sock_map using cmpxchg and xchg operations to ensure we
>   do not get stale references. Any reads into the map must be done with READ_ONCE()
>   because of this.
> 
> You initially use the READ_ONCE() on the verdict/parse/tx_msg, but later on when
> grabbing the reference you use again progs->bpf_verdict/bpf_parse/bpf_tx_msg which
> would potentially refetch it, but if updates would happen concurrently e.g. to the
> three progs, they could be NULL in the mean-time, no? bpf_prog_inc_not_zero() would
> then crash. Why are not the ones used that you fetched previously via READ_ONCE()
> for taking the ref?

Nice catch. We should use the reference fetched by READ_ONCE in all cases.

> 
> The second question I had is that verdict/parse/tx_msg are updated independently
> from each other and each could be NULL or non-NULL. What if, say, parse is NULL
> and verdict as well as tx_msg is non-NULL and the bpf_prog_inc_not_zero() on the
> tx_msg prog fails. Doesn't this cause a use-after-free since a ref on verdict wasn't
> taken earlier but the bpf_prog_put() will cause accidental misbalance/free of the
> progs?

Also good catch. I'll send patches for both now. Thanks.

> 
> It would probably help to clarify the locking comment a bit more if indeed the
> above should be okay as is.
> 
> Thanks,
> Daniel
>