netdev - Re: [bpf-next v2 1/2] bpf: skmsg, fix psock create on existing kcm/tls port

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <229054a9-a323-e8e3-5cb8-f29c8fa5f666@gmail.com>
Date:   Thu, 18 Oct 2018 13:24:40 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>, ast@...nel.org,
        daniel@...earbox.net
Cc:     netdev@...r.kernel.org
Subject: Re: [bpf-next v2 1/2] bpf: skmsg, fix psock create on existing
 kcm/tls port

On 10/18/2018 10:34 AM, Eric Dumazet wrote:
> 
> 
> On 10/17/2018 10:20 PM, John Fastabend wrote:
>> Before using the psock returned by sk_psock_get() when adding it to a
>> sockmap we need to ensure it is actually a sockmap based psock.
>> Previously we were only checking this after incrementing the reference
>> counter which was an error. This resulted in a slab-out-of-bounds
>> error when the psock was not actually a sockmap type.
>>
>> This moves the check up so the reference counter is only used
>> if it is a sockmap psock.
>>
>> Eric reported the following KASAN BUG,
>>
>> BUG: KASAN: slab-out-of-bounds in atomic_read include/asm-generic/atomic-instrumented.h:21 [inline]
>> BUG: KASAN: slab-out-of-bounds in refcount_inc_not_zero_checked+0x97/0x2f0 lib/refcount.c:120
>> Read of size 4 at addr ffff88019548be58 by task syz-executor4/22387
>>
>> CPU: 1 PID: 22387 Comm: syz-executor4 Not tainted 4.19.0-rc7+ #264
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>> Call Trace:
>>  __dump_stack lib/dump_stack.c:77 [inline]
>>  dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113
>>  print_address_description.cold.8+0x9/0x1ff mm/kasan/report.c:256
>>  kasan_report_error mm/kasan/report.c:354 [inline]
>>  kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412
>>  check_memory_region_inline mm/kasan/kasan.c:260 [inline]
>>  check_memory_region+0x13e/0x1b0 mm/kasan/kasan.c:267
>>  kasan_check_read+0x11/0x20 mm/kasan/kasan.c:272
>>  atomic_read include/asm-generic/atomic-instrumented.h:21 [inline]
>>  refcount_inc_not_zero_checked+0x97/0x2f0 lib/refcount.c:120
>>  sk_psock_get include/linux/skmsg.h:379 [inline]
>>  sock_map_link.isra.6+0x41f/0xe30 net/core/sock_map.c:178
>>  sock_hash_update_common+0x19b/0x11e0 net/core/sock_map.c:669
>>  sock_hash_update_elem+0x306/0x470 net/core/sock_map.c:738
>>  map_update_elem+0x819/0xdf0 kernel/bpf/syscall.c:818
>>
>> Signed-off-by: John Fastabend <john.fastabend@...il.com>
>> Reported-by: Eric Dumazet <eric.dumazet@...il.com>
>> Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")
>> ---


[...]

>> +static inline struct sk_psock *sk_psock_get_checked(struct sock *sk)
>> +{
>> +	struct sk_psock *psock;
>> +
>> +	rcu_read_lock();
>> +	psock = sk_psock(sk);
>> +	if (psock) {
>> +		if (sk->sk_prot->recvmsg != tcp_bpf_recvmsg) {
>> +			psock = ERR_PTR(-EBUSY);
>> +			goto out;
>> +		}
>> +
>> +		if (!refcount_inc_not_zero(&psock->refcnt))
>> +			psock = NULL;
> 
> Caller is using IS_ERR(), so you probably want to :
> 
> 			psock = ERR_PTR(-E<something>);
> 
> 

Yeah we can make this EBUSY as well. Originally I was
thinking that we could create the psock and attach it in
this case but it would be racy and require an rcu sync
most likely.

To hit this case users would need to have multiple
maps and be adding/deleting socks from those maps at
the same time. Seems pretty rare and not worth punishing
the normal case with synchronization.

Nice catch.

.John