netdev - Re: [bpf PATCH v2 1/6] bpf: sockmap, fix crash when ipv6 sock is added

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <95c027ce-e1d5-22ba-09c5-6b81504318b8@gmail.com>
Date:   Thu, 14 Jun 2018 21:46:36 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     ast@...nel.org, daniel@...earbox.net, netdev@...r.kernel.org
Subject: Re: [bpf PATCH v2 1/6] bpf: sockmap, fix crash when ipv6 sock is
 added

On 06/14/2018 04:53 PM, Martin KaFai Lau wrote:
> On Thu, Jun 14, 2018 at 09:44:46AM -0700, John Fastabend wrote:
>> This fixes a crash where we assign tcp_prot to IPv6 sockets instead
>> of tcpv6_prot.
>>
>> Previously we overwrote the sk->prot field with tcp_prot even in the
>> AF_INET6 case. This patch ensures the correct tcp_prot and tcpv6_prot
>> are used.
> 
>> Further, only allow ESTABLISHED connections to join the
>> map per note in TLS ULP,
>>
>>    /* The TLS ulp is currently supported only for TCP sockets
>>     * in ESTABLISHED state.
>>     * Supporting sockets in LISTEN state will require us
>>     * to modify the accept implementation to clone rather then
>>     * share the ulp context.
>>     */
> This bit has been moved to patch 2.

Yep better cut the comment as well.

> 
>>
>> Also tested with 'netserver -6' and 'netperf -H [IPv6]' as well as
>> 'netperf -H [IPv4]'. The ESTABLISHED check resolves the previously
>> crashing case here.
>>
>> Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support")
>> Reported-by: syzbot+5c063698bdbfac19f363@...kaller.appspotmail.com
>> Signed-off-by: John Fastabend <john.fastabend@...il.com>
>> Signed-off-by: Wei Wang <weiwan@...gle.com>
>> ---
>>  0 files changed
>>

0 files changed will fix that as well.

>> diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
>> index 52a91d8..f6dd4cd 100644
>> --- a/kernel/bpf/sockmap.c
>> +++ b/kernel/bpf/sockmap.c
>> @@ -140,6 +140,7 @@ static int bpf_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>>  static int bpf_tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size);
>>  static int bpf_tcp_sendpage(struct sock *sk, struct page *page,
>>  			    int offset, size_t size, int flags);
>> +static void bpf_tcp_close(struct sock *sk, long timeout);
>>  
>>  static inline struct smap_psock *smap_psock_sk(const struct sock *sk)
>>  {
>> @@ -161,7 +162,42 @@ static bool bpf_tcp_stream_read(const struct sock *sk)
>>  	return !empty;
>>  }
>>  
>> -static struct proto tcp_bpf_proto;
>> +enum {
>> +	SOCKMAP_IPV4,
>> +	SOCKMAP_IPV6,
>> +	SOCKMAP_NUM_PROTS,
>> +};
>> +
>> +enum {
>> +	SOCKMAP_BASE,
>> +	SOCKMAP_TX,
>> +	SOCKMAP_NUM_CONFIGS,
>> +};
>> +
>> +static struct proto *saved_tcpv6_prot;
> __read_mostly
> 

Sure makes sense.

>> +static DEFINE_MUTEX(tcpv6_prot_mutex);
>> +static struct proto bpf_tcp_prots[SOCKMAP_NUM_PROTS][SOCKMAP_NUM_CONFIGS];
>> +static void build_protos(struct proto prot[SOCKMAP_NUM_CONFIGS],
>> +			 struct proto *base)
>> +{
>> +	prot[SOCKMAP_BASE]			= *base;
>> +	prot[SOCKMAP_BASE].close		= bpf_tcp_close;
>> +	prot[SOCKMAP_BASE].recvmsg		= bpf_tcp_recvmsg;
>> +	prot[SOCKMAP_BASE].stream_memory_read	= bpf_tcp_stream_read;
>> +
>> +	prot[SOCKMAP_TX]			= prot[SOCKMAP_BASE];
>> +	prot[SOCKMAP_TX].sendmsg		= bpf_tcp_sendmsg;
>> +	prot[SOCKMAP_TX].sendpage		= bpf_tcp_sendpage;
>> +}
>> +
>> +static void update_sk_prot(struct sock *sk, struct smap_psock *psock)
>> +{
>> +	int family = sk->sk_family == AF_INET6 ? SOCKMAP_IPV6 : SOCKMAP_IPV4;
>> +	int conf = psock->bpf_tx_msg ? SOCKMAP_TX : SOCKMAP_BASE;
>> +
>> +	sk->sk_prot = &bpf_tcp_prots[family][conf];
>> +}
>> +
>>  static int bpf_tcp_init(struct sock *sk)
>>  {
>>  	struct smap_psock *psock;
>> @@ -181,14 +217,17 @@ static int bpf_tcp_init(struct sock *sk)
>>  	psock->save_close = sk->sk_prot->close;
>>  	psock->sk_proto = sk->sk_prot;
>>  
>> -	if (psock->bpf_tx_msg) {
>> -		tcp_bpf_proto.sendmsg = bpf_tcp_sendmsg;
>> -		tcp_bpf_proto.sendpage = bpf_tcp_sendpage;
>> -		tcp_bpf_proto.recvmsg = bpf_tcp_recvmsg;
>> -		tcp_bpf_proto.stream_memory_read = bpf_tcp_stream_read;
>> +	/* Build IPv6 sockmap whenever the address of tcpv6_prot changes */
>> +	if (sk->sk_family == AF_INET6 &&
>> +	    unlikely(sk->sk_prot != smp_load_acquire(&saved_tcpv6_prot))) {
>> +		mutex_lock(&tcpv6_prot_mutex);
> bpf_tcp_init() can be called by skops?
> Can mutex_lock() be used here?
> 

No mutex lock can not be used here. Both are called
with rcu_read_lock() and we can not sleep. Thanks
for catching. Also this will give a kernel splat now
that I have the right config options. Guess we need
a v3 :/

Thanks,
John