From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [PATCH bpf 1/2] bpf, sockmap: fix potential use after free in bpf_tcp_close Date: Fri, 24 Aug 2018 17:25:57 -0700 Message-ID: <42a094b6-baaa-6d03-8e25-9ec7e15cee30@gmail.com> References: <20180824200851.12308-1-daniel@iogearbox.net> <20180824200851.12308-2-daniel@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org To: Daniel Borkmann , alexei.starovoitov@gmail.com Return-path: Received: from mail-io0-f193.google.com ([209.85.223.193]:39353 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725737AbeHYEEs (ORCPT ); Sat, 25 Aug 2018 00:04:48 -0400 Received: by mail-io0-f193.google.com with SMTP id l7-v6so8508656iok.6 for ; Fri, 24 Aug 2018 17:27:53 -0700 (PDT) In-Reply-To: <20180824200851.12308-2-daniel@iogearbox.net> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 08/24/2018 01:08 PM, Daniel Borkmann wrote: > bpf_tcp_close() we pop the psock linkage to a map via psock_map_pop(). > A parallel update on the sock hash map can happen between psock_map_pop() > and lookup_elem_raw() where we override the element under link->hash / > link->key. In bpf_tcp_close()'s lookup_elem_raw() we subsequently only > test whether an element is present, but we do not test whether the > element is infact the element we were looking for. > > We lock the sock in bpf_tcp_close() during that time, so do we hold > the lock in sock_hash_update_elem(). However, the latter locks the > sock which is newly updated, not the one we're purging from the hash > table. This means that while one CPU is doing the lookup from bpf_tcp_close(), > another CPU is doing the map update in parallel, dropped our sock from > the hlist and released the psock. > > Subsequently the first CPU will find the new sock and attempts to drop > and release the old sock yet another time. Fix is that we need to check > the elements for a match after lookup, similar as we do in the sock map. > Note that the hash tab elems are freed via RCU, so access to their > link->hash / link->key is fine since we're under RCU read side there. > > Fixes: e9db4ef6bf4c ("bpf: sockhash fix omitted bucket lock in sock_close") > Signed-off-by: Daniel Borkmann > --- Looks good to me, nice catch by the way. Acked-by: John Fastabend