netdev - Re: [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case

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: <f25d57ac-6a02-17e3-877b-365bd446e102@gmail.com>
Date:   Thu, 5 Jul 2018 07:41:29 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org
Cc:     netdev@...r.kernel.org, kafai@...com
Subject: Re: [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in
 multi-map case

On 07/03/2018 07:40 AM, Daniel Borkmann wrote:
> On 06/30/2018 03:51 PM, John Fastabend wrote:
>> The current code, in the error path of sock_hash_ctx_update_elem,
>> checks if the sock has a psock in the user data and if so decrements
>> the reference count of the psock. However, if the error happens early
>> in the error path we may have never incremented the psock reference
>> count and if the psock exists because the sock is in another map then
>> we may inadvertently decrement the reference count.
>>
>> Fix this by making the error path only call smap_release_sock if the
>> error happens after the increment.
>>
>> Reported-by: syzbot+d464d2c20c717ef5a6a8@...kaller.appspotmail.com
>> Fixes: 81110384441a ("bpf: sockmap, add hash map support")
>> Signed-off-by: John Fastabend <john.fastabend@...il.com>
>> ---

[...]

>> @@ -2324,7 +2324,12 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
>>  	if (err)
>>  		goto err;
>>  
>> -	/* bpf_map_update_elem() can be called in_irq() */
>> +	psock = smap_psock_sk(sock);
>> +	if (unlikely(!psock)) {
>> +		err = -EINVAL;
>> +		goto err;
>> +	}
> 
> Is an error even possible at this point? If __sock_map_ctx_update_elem() succeeds,
> we either allocated and linked a new psock to the sock or we inc'ed the existing
> one's refcount. From my reading it seems we should always succeed the subsequent
> smap_psock_sk(). If we would have failed here in between it would mean we'd have
> a refcount imbalance somewhere?
> 

Its not possible will replace with a comment. Thanks.