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  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:   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
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux - Powered by OpenVZ