netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added
@ 2018-06-01 19:46 John Fastabend
  2018-06-01 19:58 ` Eric Dumazet
  0 siblings, 1 reply; 6+ messages in thread
From: John Fastabend @ 2018-06-01 19:46 UTC (permalink / raw)
  To: edumazet, ast, daniel; +Cc: netdev

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.
    */

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@syzkaller.appspotmail.com
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Wei Wang <weiwan@google.com>
---
 kernel/bpf/sockmap.c |   23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 95a84b2..1c8bf18 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -41,6 +41,7 @@
 #include <linux/mm.h>
 #include <net/strparser.h>
 #include <net/tcp.h>
+#include <net/transp_v6.h>
 #include <linux/ptr_ring.h>
 #include <net/inet_common.h>
 #include <linux/sched/signal.h>
@@ -134,6 +135,8 @@ static bool bpf_tcp_stream_read(const struct sock *sk)
 }
 
 static struct proto tcp_bpf_proto;
+static struct proto tcpv6_bpf_proto;
+
 static int bpf_tcp_init(struct sock *sk)
 {
 	struct smap_psock *psock;
@@ -154,13 +157,21 @@ static int bpf_tcp_init(struct sock *sk)
 	psock->sk_proto = sk->sk_prot;
 
 	if (psock->bpf_tx_msg) {
+		tcpv6_bpf_proto.sendmsg = bpf_tcp_sendmsg;
+		tcpv6_bpf_proto.sendpage = bpf_tcp_sendpage;
+		tcpv6_bpf_proto.recvmsg = bpf_tcp_recvmsg;
+		tcpv6_bpf_proto.stream_memory_read = bpf_tcp_stream_read;
 		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;
 	}
 
-	sk->sk_prot = &tcp_bpf_proto;
+	if (sk->sk_family == AF_INET6)
+		sk->sk_prot = &tcpv6_bpf_proto;
+	else
+		sk->sk_prot = &tcp_bpf_proto;
+
 	rcu_read_unlock();
 	return 0;
 }
@@ -1072,6 +1083,8 @@ static int bpf_tcp_ulp_register(void)
 {
 	tcp_bpf_proto = tcp_prot;
 	tcp_bpf_proto.close = bpf_tcp_close;
+	tcpv6_bpf_proto = tcpv6_prot;
+	tcpv6_bpf_proto.close = bpf_tcp_close;
 	/* Once BPF TX ULP is registered it is never unregistered. It
 	 * will be in the ULP list for the lifetime of the system. Doing
 	 * duplicate registers is not a problem.
@@ -1689,6 +1702,14 @@ static int sock_map_ctx_update_elem(struct bpf_sock_ops_kern *skops,
 
 	sock = skops->sk;
 
+	/* ULPs are 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.
+	 */
+	if (sock->sk_state != TCP_ESTABLISHED)
+		return -ENOTSUPP;
+
 	/* 1. If sock map has BPF programs those will be inherited by the
 	 * sock being added. If the sock is already attached to BPF programs
 	 * this results in an error.

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added
  2018-06-01 19:46 [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added John Fastabend
@ 2018-06-01 19:58 ` Eric Dumazet
  2018-06-02 21:39   ` John Fastabend
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2018-06-01 19:58 UTC (permalink / raw)
  To: John Fastabend, edumazet, ast, daniel; +Cc: netdev



On 06/01/2018 03:46 PM, John Fastabend wrote:
> This fixes a crash where we assign tcp_prot to IPv6 sockets instead
> of tcpv6_prot.

...

> +	/* ULPs are 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.
> +	 */
> +	if (sock->sk_state != TCP_ESTABLISHED)
> +		return -ENOTSUPP;
> +
>  	/* 1. If sock map has BPF programs those will be inherited by the
>  	 * sock being added. If the sock is already attached to BPF programs
>  	 * this results in an error.
> 

Next question will be then : What happens if syzbot uses tcp_disconnect() and then listen() ?

Thanks !

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added
  2018-06-01 19:58 ` Eric Dumazet
@ 2018-06-02 21:39   ` John Fastabend
  2018-06-04 13:39     ` Daniel Borkmann
  0 siblings, 1 reply; 6+ messages in thread
From: John Fastabend @ 2018-06-02 21:39 UTC (permalink / raw)
  To: Eric Dumazet, edumazet, ast, daniel, Dave Watson; +Cc: netdev

On 06/01/2018 12:58 PM, Eric Dumazet wrote:
> 
> 
> On 06/01/2018 03:46 PM, John Fastabend wrote:
>> This fixes a crash where we assign tcp_prot to IPv6 sockets instead
>> of tcpv6_prot.
> 
> ...
> 
>> +	/* ULPs are 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.
>> +	 */
>> +	if (sock->sk_state != TCP_ESTABLISHED)
>> +		return -ENOTSUPP;
>> +
>>  	/* 1. If sock map has BPF programs those will be inherited by the
>>  	 * sock being added. If the sock is already attached to BPF programs
>>  	 * this results in an error.
>>
> 
> Next question will be then : What happens if syzbot uses tcp_disconnect() and then listen() ?

Yep we need to fix that as well :( Looks like we can plumb the
unhash callback and remove it from the sockmap when the socket
goes through tcp_disconnect().

This patch should go in as-is though and we can fix the disconnect
issue with a new patch.

Adding Dave Watson to the thread as well because I'm guessing
the disconnect() case is also applicable to TLS. At least I see
a hw handler for unhash but there does not appear to be a handler
in the SW case, at least from a quick glance.

Thanks again!

> 
> Thanks !
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added
  2018-06-02 21:39   ` John Fastabend
@ 2018-06-04 13:39     ` Daniel Borkmann
  2018-06-04 13:57       ` John Fastabend
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2018-06-04 13:39 UTC (permalink / raw)
  To: John Fastabend, Eric Dumazet, edumazet, ast, Dave Watson; +Cc: netdev

Hey guys,

On 06/02/2018 11:39 PM, John Fastabend wrote:
> On 06/01/2018 12:58 PM, Eric Dumazet wrote:
>> On 06/01/2018 03:46 PM, John Fastabend wrote:
>>> This fixes a crash where we assign tcp_prot to IPv6 sockets instead
>>> of tcpv6_prot.
>>
>> ...
>>
>>> +	/* ULPs are 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.
>>> +	 */
>>> +	if (sock->sk_state != TCP_ESTABLISHED)
>>> +		return -ENOTSUPP;
>>> +
>>>  	/* 1. If sock map has BPF programs those will be inherited by the
>>>  	 * sock being added. If the sock is already attached to BPF programs
>>>  	 * this results in an error.
>>
>> Next question will be then : What happens if syzbot uses tcp_disconnect() and then listen() ?
> 
> Yep we need to fix that as well :( Looks like we can plumb the
> unhash callback and remove it from the sockmap when the socket
> goes through tcp_disconnect().
> 
> This patch should go in as-is though and we can fix the disconnect
> issue with a new patch.
> 
> Adding Dave Watson to the thread as well because I'm guessing
> the disconnect() case is also applicable to TLS. At least I see
> a hw handler for unhash but there does not appear to be a handler
> in the SW case, at least from a quick glance.
> 
> Thanks again!

Given the discussion and fixes weren't resolved resp. ready in time for 4.17,
and last bpf pr for it went out last week, we need to route this via -stable
once all is hashed out.

This fix here therefore needs to be rebased against bpf-next tree, and as far
as I can see another fix for hash map is also needed to address the same issue.

After that, likely also fixes for the disconnect + listen case are needed.

(I can use the one here later on for -stable backport, but given merge window
is open this needs a rebase and a resolution for hash map.)

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added
  2018-06-04 13:39     ` Daniel Borkmann
@ 2018-06-04 13:57       ` John Fastabend
  2018-06-04 14:55         ` Daniel Borkmann
  0 siblings, 1 reply; 6+ messages in thread
From: John Fastabend @ 2018-06-04 13:57 UTC (permalink / raw)
  To: Daniel Borkmann, Eric Dumazet, edumazet, ast, Dave Watson; +Cc: netdev

On 06/04/2018 06:39 AM, Daniel Borkmann wrote:
> Hey guys,
> 
> On 06/02/2018 11:39 PM, John Fastabend wrote:
>> On 06/01/2018 12:58 PM, Eric Dumazet wrote:
>>> On 06/01/2018 03:46 PM, John Fastabend wrote:
>>>> This fixes a crash where we assign tcp_prot to IPv6 sockets instead
>>>> of tcpv6_prot.
>>>
>>> ...
>>>
>>>> +	/* ULPs are 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.
>>>> +	 */
>>>> +	if (sock->sk_state != TCP_ESTABLISHED)
>>>> +		return -ENOTSUPP;
>>>> +
>>>>  	/* 1. If sock map has BPF programs those will be inherited by the
>>>>  	 * sock being added. If the sock is already attached to BPF programs
>>>>  	 * this results in an error.
>>>
>>> Next question will be then : What happens if syzbot uses tcp_disconnect() and then listen() ?
>>
>> Yep we need to fix that as well :( Looks like we can plumb the
>> unhash callback and remove it from the sockmap when the socket
>> goes through tcp_disconnect().
>>
>> This patch should go in as-is though and we can fix the disconnect
>> issue with a new patch.
>>
>> Adding Dave Watson to the thread as well because I'm guessing
>> the disconnect() case is also applicable to TLS. At least I see
>> a hw handler for unhash but there does not appear to be a handler
>> in the SW case, at least from a quick glance.
>>
>> Thanks again!
> 
> Given the discussion and fixes weren't resolved resp. ready in time for 4.17,
> and last bpf pr for it went out last week, we need to route this via -stable
> once all is hashed out.
> 

OK.

> This fix here therefore needs to be rebased against bpf-next tree, and as far
> as I can see another fix for hash map is also needed to address the same issue.
> 

This fix works for both sockmap and sockhash because they use the same
ulp register and init paths. But, will rebase for net-next and send out
this morning.

> After that, likely also fixes for the disconnect + listen case are needed.
> 

Yep will have a fix today for this.

> (I can use the one here later on for -stable backport, but given merge window
> is open this needs a rebase and a resolution for hash map.)
> 

hash map is also resolved with the same patch but please do queue this
up for -stable.


> Thanks,
> Daniel
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added
  2018-06-04 13:57       ` John Fastabend
@ 2018-06-04 14:55         ` Daniel Borkmann
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2018-06-04 14:55 UTC (permalink / raw)
  To: John Fastabend, Eric Dumazet, edumazet, ast, Dave Watson; +Cc: netdev

On 06/04/2018 03:57 PM, John Fastabend wrote:
> On 06/04/2018 06:39 AM, Daniel Borkmann wrote:
>> On 06/02/2018 11:39 PM, John Fastabend wrote:
>>> On 06/01/2018 12:58 PM, Eric Dumazet wrote:
>>>> On 06/01/2018 03:46 PM, John Fastabend wrote:
>>>>> This fixes a crash where we assign tcp_prot to IPv6 sockets instead
>>>>> of tcpv6_prot.
>>>>
>>>> ...
>>>>
>>>>> +	/* ULPs are 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.
>>>>> +	 */
>>>>> +	if (sock->sk_state != TCP_ESTABLISHED)
>>>>> +		return -ENOTSUPP;
>>>>> +
>>>>>  	/* 1. If sock map has BPF programs those will be inherited by the
>>>>>  	 * sock being added. If the sock is already attached to BPF programs
>>>>>  	 * this results in an error.
>>>>
>>>> Next question will be then : What happens if syzbot uses tcp_disconnect() and then listen() ?
>>>
>>> Yep we need to fix that as well :( Looks like we can plumb the
>>> unhash callback and remove it from the sockmap when the socket
>>> goes through tcp_disconnect().
>>>
>>> This patch should go in as-is though and we can fix the disconnect
>>> issue with a new patch.
>>>
>>> Adding Dave Watson to the thread as well because I'm guessing
>>> the disconnect() case is also applicable to TLS. At least I see
>>> a hw handler for unhash but there does not appear to be a handler
>>> in the SW case, at least from a quick glance.
>>>
>>> Thanks again!
>>
>> Given the discussion and fixes weren't resolved resp. ready in time for 4.17,
>> and last bpf pr for it went out last week, we need to route this via -stable
>> once all is hashed out.
> 
> OK.
> 
>> This fix here therefore needs to be rebased against bpf-next tree, and as far
>> as I can see another fix for hash map is also needed to address the same issue.
>>
> 
> This fix works for both sockmap and sockhash because they use the same
> ulp register and init paths. But, will rebase for net-next and send out
> this morning.

Ok, right, because in bpf-next this eventually goes into __sock_map_ctx_update_elem()
instead of sock_map_ctx_update_elem() call site.

>> After that, likely also fixes for the disconnect + listen case are needed.
> 
> Yep will have a fix today for this.
> 
>> (I can use the one here later on for -stable backport, but given merge window
>> is open this needs a rebase and a resolution for hash map.)
> 
> hash map is also resolved with the same patch but please do queue this
> up for -stable.

Will do, thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-06-04 14:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-01 19:46 [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added John Fastabend
2018-06-01 19:58 ` Eric Dumazet
2018-06-02 21:39   ` John Fastabend
2018-06-04 13:39     ` Daniel Borkmann
2018-06-04 13:57       ` John Fastabend
2018-06-04 14:55         ` Daniel Borkmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).