[PATCH bpf v2 0/4] Bugfixes for syncookie BPF helpers

[PATCH bpf v2 0/4] Bugfixes for syncookie BPF helpers

[Maxim Mikityanskiy]

This series contains generic bugfixes for the syncookie BPF helpers. It
used to be part of series [1], but has been separated to apply to the
bpf branch as fixes.

[1]: https://lore.kernel.org/bpf/20211020095815.GJ28644@breakpoint.cc/t/

Maxim Mikityanskiy (4):
  bpf: Use ipv6_only_sock in bpf_tcp_gen_syncookie
  bpf: Support dual-stack sockets in bpf_tcp_check_syncookie
  bpf: Use EOPNOTSUPP in bpf_tcp_check_syncookie
  bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie

 include/uapi/linux/bpf.h       |  6 ++++--
 net/core/filter.c              | 21 +++++++++++++++------
 tools/include/uapi/linux/bpf.h |  6 ++++--
 3 files changed, 23 insertions(+), 10 deletions(-)

-- 
2.30.2

[PATCH bpf v2 1/4] bpf: Use ipv6_only_sock in bpf_tcp_gen_syncookie

[Maxim Mikityanskiy]

Instead of querying the sk_ipv6only field directly, use the dedicated
ipv6_only_sock helper.

Fixes: 70d66244317e ("bpf: add bpf_tcp_gen_syncookie helper")
Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
---
 net/core/filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 4603b7cd3cd1..05efa691b796 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6848,7 +6848,7 @@ BPF_CALL_5(bpf_tcp_gen_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
 	 */
 	switch (((struct iphdr *)iph)->version) {
 	case 4:
-		if (sk->sk_family == AF_INET6 && sk->sk_ipv6only)
+		if (sk->sk_family == AF_INET6 && ipv6_only_sock(sk))
 			return -EINVAL;
 
 		mss = tcp_v4_get_syncookie(sk, iph, th, &cookie);
-- 
2.30.2

[PATCH bpf v2 2/4] bpf: Support dual-stack sockets in bpf_tcp_check_syncookie

[Maxim Mikityanskiy]

bpf_tcp_gen_syncookie looks at the IP version in the IP header and
validates the address family of the socket. It supports IPv4 packets in
AF_INET6 dual-stack sockets.

On the other hand, bpf_tcp_check_syncookie looks only at the address
family of the socket, ignoring the real IP version in headers, and
validates only the packet size. This implementation has some drawbacks:

1. Packets are not validated properly, allowing a BPF program to trick
   bpf_tcp_check_syncookie into handling an IPv6 packet on an IPv4
   socket.

2. Dual-stack sockets fail the checks on IPv4 packets. IPv4 clients end
   up receiving a SYNACK with the cookie, but the following ACK gets
   dropped.

This patch fixes these issues by changing the checks in
bpf_tcp_check_syncookie to match the ones in bpf_tcp_gen_syncookie. IP
version from the header is taken into account, and it is validated
properly with address family.

Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie")
Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
---
 net/core/filter.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 05efa691b796..780e635fb52a 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6774,24 +6774,33 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
 	if (!th->ack || th->rst || th->syn)
 		return -ENOENT;
 
+	if (unlikely(iph_len < sizeof(struct iphdr)))
+		return -EINVAL;
+
 	if (tcp_synq_no_recent_overflow(sk))
 		return -ENOENT;
 
 	cookie = ntohl(th->ack_seq) - 1;
 
-	switch (sk->sk_family) {
-	case AF_INET:
-		if (unlikely(iph_len < sizeof(struct iphdr)))
+	/* Both struct iphdr and struct ipv6hdr have the version field at the
+	 * same offset so we can cast to the shorter header (struct iphdr).
+	 */
+	switch (((struct iphdr *)iph)->version) {
+	case 4:
+		if (sk->sk_family == AF_INET6 && ipv6_only_sock(sk))
 			return -EINVAL;
 
 		ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
 		break;
 
 #if IS_BUILTIN(CONFIG_IPV6)
-	case AF_INET6:
+	case 6:
 		if (unlikely(iph_len < sizeof(struct ipv6hdr)))
 			return -EINVAL;
 
+		if (sk->sk_family != AF_INET6)
+			return -EINVAL;
+
 		ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
 		break;
 #endif /* CONFIG_IPV6 */
-- 
2.30.2

[PATCH bpf v2 3/4] bpf: Use EOPNOTSUPP in bpf_tcp_check_syncookie

[Maxim Mikityanskiy]

When CONFIG_SYN_COOKIES is off, bpf_tcp_check_syncookie returns
ENOTSUPP. It's a non-standard and deprecated code. The related function
bpf_tcp_gen_syncookie and most of the other functions use EOPNOTSUPP if
some feature is not available. This patch changes ENOTSUPP to EOPNOTSUPP
in bpf_tcp_check_syncookie.

Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie")
Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
---
 net/core/filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 780e635fb52a..2c9106704821 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6814,7 +6814,7 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
 
 	return -ENOENT;
 #else
-	return -ENOTSUPP;
+	return -EOPNOTSUPP;
 #endif
 }
 
-- 
2.30.2

[PATCH bpf v2 4/4] bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie

[Maxim Mikityanskiy]

bpf_tcp_gen_syncookie and bpf_tcp_check_syncookie expect the full length
of the TCP header (with all extensions). Fix the documentation that says
it should be sizeof(struct tcphdr).

Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie")
Fixes: 70d66244317e ("bpf: add bpf_tcp_gen_syncookie helper")
Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
---
 include/uapi/linux/bpf.h       | 6 ++++--
 tools/include/uapi/linux/bpf.h | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index b0383d371b9a..520f1e557dce 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3553,7 +3553,8 @@ union bpf_attr {
  * 		**sizeof**\ (**struct ip6hdr**).
  *
  * 		*th* points to the start of the TCP header, while *th_len*
- * 		contains **sizeof**\ (**struct tcphdr**).
+ *		contains the length of the TCP header (at least
+ *		**sizeof**\ (**struct tcphdr**)).
  * 	Return
  * 		0 if *iph* and *th* are a valid SYN cookie ACK, or a negative
  * 		error otherwise.
@@ -3739,7 +3740,8 @@ union bpf_attr {
  *		**sizeof**\ (**struct ip6hdr**).
  *
  *		*th* points to the start of the TCP header, while *th_len*
- *		contains the length of the TCP header.
+ *		contains the length of the TCP header (at least
+ *		**sizeof**\ (**struct tcphdr**)).
  *	Return
  *		On success, lower 32 bits hold the generated SYN cookie in
  *		followed by 16 bits which hold the MSS value for that cookie,
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index b0383d371b9a..520f1e557dce 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3553,7 +3553,8 @@ union bpf_attr {
  * 		**sizeof**\ (**struct ip6hdr**).
  *
  * 		*th* points to the start of the TCP header, while *th_len*
- * 		contains **sizeof**\ (**struct tcphdr**).
+ *		contains the length of the TCP header (at least
+ *		**sizeof**\ (**struct tcphdr**)).
  * 	Return
  * 		0 if *iph* and *th* are a valid SYN cookie ACK, or a negative
  * 		error otherwise.
@@ -3739,7 +3740,8 @@ union bpf_attr {
  *		**sizeof**\ (**struct ip6hdr**).
  *
  *		*th* points to the start of the TCP header, while *th_len*
- *		contains the length of the TCP header.
+ *		contains the length of the TCP header (at least
+ *		**sizeof**\ (**struct tcphdr**)).
  *	Return
  *		On success, lower 32 bits hold the generated SYN cookie in
  *		followed by 16 bits which hold the MSS value for that cookie,
-- 
2.30.2

RE: [PATCH bpf v2 1/4] bpf: Use ipv6_only_sock in bpf_tcp_gen_syncookie

[John Fastabend]

Maxim Mikityanskiy wrote:
> Instead of querying the sk_ipv6only field directly, use the dedicated
> ipv6_only_sock helper.
> 
> Fixes: 70d66244317e ("bpf: add bpf_tcp_gen_syncookie helper")
> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
> ---

Not really a fix, but LGTM.

Acked-by: John Fastabend <john.fastabend@gmail.com>

RE: [PATCH bpf v2 2/4] bpf: Support dual-stack sockets in bpf_tcp_check_syncookie

[John Fastabend]

Maxim Mikityanskiy wrote:
> bpf_tcp_gen_syncookie looks at the IP version in the IP header and
> validates the address family of the socket. It supports IPv4 packets in
> AF_INET6 dual-stack sockets.
> 
> On the other hand, bpf_tcp_check_syncookie looks only at the address
> family of the socket, ignoring the real IP version in headers, and
> validates only the packet size. This implementation has some drawbacks:
> 
> 1. Packets are not validated properly, allowing a BPF program to trick
>    bpf_tcp_check_syncookie into handling an IPv6 packet on an IPv4
>    socket.

These programs are all CAP_NET_ADMIN I believe so not so sure this is
critical from a BPF program might trick the helper, but consistency
is nice.

> 
> 2. Dual-stack sockets fail the checks on IPv4 packets. IPv4 clients end
>    up receiving a SYNACK with the cookie, but the following ACK gets
>    dropped.

Agree we need to fix this. Also would be nice to add a test to capture
this case so we don't break it again later. Its a bit subtle so might
not be caught right away without a selftest.

> 
> This patch fixes these issues by changing the checks in
> bpf_tcp_check_syncookie to match the ones in bpf_tcp_gen_syncookie. IP
> version from the header is taken into account, and it is validated
> properly with address family.

Code looks good, would be nice to have a test.

Acked-by: John Fastabend <john.fastabend@gmail.com>

> 
> Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie")
> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
> ---
>  net/core/filter.c | 17 +++++++++++++----

RE: [PATCH bpf v2 3/4] bpf: Use EOPNOTSUPP in bpf_tcp_check_syncookie

[John Fastabend]

Maxim Mikityanskiy wrote:
> When CONFIG_SYN_COOKIES is off, bpf_tcp_check_syncookie returns
> ENOTSUPP. It's a non-standard and deprecated code. The related function
> bpf_tcp_gen_syncookie and most of the other functions use EOPNOTSUPP if
> some feature is not available. This patch changes ENOTSUPP to EOPNOTSUPP
> in bpf_tcp_check_syncookie.
> 
> Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie")
> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>

This came up in another thread? Or was it the same and we lost the context
in the commit msg. Either way I don't think we should start one-off
changing these user facing error codes. Its not the only spot we do this
and its been this way for sometime.

Is it causing a real problem?

> ---
>  net/core/filter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 780e635fb52a..2c9106704821 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -6814,7 +6814,7 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
>  
>  	return -ENOENT;
>  #else
> -	return -ENOTSUPP;
> +	return -EOPNOTSUPP;
>  #endif
>  }
>  
> -- 
> 2.30.2
> 

RE: [PATCH bpf v2 4/4] bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie

[John Fastabend]

Maxim Mikityanskiy wrote:
> bpf_tcp_gen_syncookie and bpf_tcp_check_syncookie expect the full length
> of the TCP header (with all extensions). Fix the documentation that says
> it should be sizeof(struct tcphdr).
> 
> Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie")
> Fixes: 70d66244317e ("bpf: add bpf_tcp_gen_syncookie helper")
> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>

Not sure I would push doc fixes at bpf tree, would be fine to go
through bpf-next. But change looks good.

Acked-by: John Fastabend <john.fastabend@gmail.com>

RE: [PATCH bpf v2 0/4] Bugfixes for syncookie BPF helpers

[John Fastabend]

Maxim Mikityanskiy wrote:
> This series contains generic bugfixes for the syncookie BPF helpers. It
> used to be part of series [1], but has been separated to apply to the
> bpf branch as fixes.

2/4 looks like the only real fix here. I think it would be more effective
to push 2/4 with a selftest and get that out of the way. The others are
nice but could go via bpf-next imo and 3/4 can likely be dropped imo. 

Thanks for the fix.
John

Re: [PATCH bpf v2 4/4] bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie

[Lorenz Bauer]

On Mon, 24 Jan 2022 at 15:13, Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
>
> bpf_tcp_gen_syncookie and bpf_tcp_check_syncookie expect the full length
> of the TCP header (with all extensions). Fix the documentation that says
> it should be sizeof(struct tcphdr).

I don't understand this change, sorry. Are you referring to the fact
that the check is len < sizeof(*th) instead of len != sizeof(*th)?

Your commit message makes me think that the helpers will access data
in the extension headers, which isn't true as far as I can tell. That
would be a problem in fact, since it could be used to read memory that
the verifier hasn't deemed safe.

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf v2 1/4] bpf: Use ipv6_only_sock in bpf_tcp_gen_syncookie

[Lorenz Bauer]

On Mon, 24 Jan 2022 at 15:13, Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
>
> Instead of querying the sk_ipv6only field directly, use the dedicated
> ipv6_only_sock helper.
>
> Fixes: 70d66244317e ("bpf: add bpf_tcp_gen_syncookie helper")
> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>

Acked-by: Lorenz Bauer <lmb@cloudflare.com>

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf v2 2/4] bpf: Support dual-stack sockets in bpf_tcp_check_syncookie

[Lorenz Bauer]

On Mon, 24 Jan 2022 at 15:13, Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
>
> bpf_tcp_gen_syncookie looks at the IP version in the IP header and
> validates the address family of the socket. It supports IPv4 packets in
> AF_INET6 dual-stack sockets.
>
> On the other hand, bpf_tcp_check_syncookie looks only at the address
> family of the socket, ignoring the real IP version in headers, and
> validates only the packet size. This implementation has some drawbacks:
>
> 1. Packets are not validated properly, allowing a BPF program to trick
>    bpf_tcp_check_syncookie into handling an IPv6 packet on an IPv4
>    socket.
>
> 2. Dual-stack sockets fail the checks on IPv4 packets. IPv4 clients end
>    up receiving a SYNACK with the cookie, but the following ACK gets
>    dropped.
>
> This patch fixes these issues by changing the checks in
> bpf_tcp_check_syncookie to match the ones in bpf_tcp_gen_syncookie. IP
> version from the header is taken into account, and it is validated
> properly with address family.
>
> Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie")
> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
> ---
>  net/core/filter.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 05efa691b796..780e635fb52a 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -6774,24 +6774,33 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
>         if (!th->ack || th->rst || th->syn)
>                 return -ENOENT;
>
> +       if (unlikely(iph_len < sizeof(struct iphdr)))
> +               return -EINVAL;
> +
>         if (tcp_synq_no_recent_overflow(sk))
>                 return -ENOENT;
>
>         cookie = ntohl(th->ack_seq) - 1;
>
> -       switch (sk->sk_family) {
> -       case AF_INET:
> -               if (unlikely(iph_len < sizeof(struct iphdr)))
> +       /* Both struct iphdr and struct ipv6hdr have the version field at the
> +        * same offset so we can cast to the shorter header (struct iphdr).
> +        */
> +       switch (((struct iphdr *)iph)->version) {
> +       case 4:
> +               if (sk->sk_family == AF_INET6 && ipv6_only_sock(sk))
>                         return -EINVAL;

Wouldn't this allow an arbitrary value for sk->sk_family, since there
is no further check that sk_family is AF_INET?

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf v2 1/4] bpf: Use ipv6_only_sock in bpf_tcp_gen_syncookie

[Petar Penkov]

On Wed, Jan 26, 2022 at 1:46 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Mon, 24 Jan 2022 at 15:13, Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
> >
> > Instead of querying the sk_ipv6only field directly, use the dedicated
> > ipv6_only_sock helper.
> >
> > Fixes: 70d66244317e ("bpf: add bpf_tcp_gen_syncookie helper")
> > Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> > Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
>
> Acked-by: Lorenz Bauer <lmb@cloudflare.com>

Acked-by: Petar Penkov <ppenkov@google.com>

Re: [PATCH bpf v2 3/4] bpf: Use EOPNOTSUPP in bpf_tcp_check_syncookie

[Maxim Mikityanskiy]

On 2022-01-25 09:06, John Fastabend wrote:
> Maxim Mikityanskiy wrote:
>> When CONFIG_SYN_COOKIES is off, bpf_tcp_check_syncookie returns
>> ENOTSUPP. It's a non-standard and deprecated code. The related function
>> bpf_tcp_gen_syncookie and most of the other functions use EOPNOTSUPP if
>> some feature is not available. This patch changes ENOTSUPP to EOPNOTSUPP
>> in bpf_tcp_check_syncookie.
>>
>> Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie")
>> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
>> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
> 
> This came up in another thread? Or was it the same and we lost the context
> in the commit msg. Either way I don't think we should start one-off
> changing these user facing error codes. Its not the only spot we do this
> and its been this way for sometime.
> 
> Is it causing a real problem?

I'm not aware of anyone complaining about it. It's just a cleanup to use 
the proper error code, since ENOTSUPP is a non-standard one (used in 
NFS?), for example, strerror() returns "Unknown error 524" instead of 
"Operation not supported".

Source: Documentation/dev-tools/checkpatch.rst:

 > ENOTSUPP is not a standard error code and should be avoided in new
 > patches. EOPNOTSUPP should be used instead.
 >
 > See: https://lore.kernel.org/netdev/20200510182252.GA411829@lunn.ch/

>> ---
>>   net/core/filter.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 780e635fb52a..2c9106704821 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -6814,7 +6814,7 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
>>   
>>   	return -ENOENT;
>>   #else
>> -	return -ENOTSUPP;
>> +	return -EOPNOTSUPP;
>>   #endif
>>   }
>>   
>> -- 
>> 2.30.2
>>

Re: [PATCH bpf v2 4/4] bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie

[Maxim Mikityanskiy]

On 2022-01-26 11:45, Lorenz Bauer wrote:
> On Mon, 24 Jan 2022 at 15:13, Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
>>
>> bpf_tcp_gen_syncookie and bpf_tcp_check_syncookie expect the full length
>> of the TCP header (with all extensions). Fix the documentation that says
>> it should be sizeof(struct tcphdr).
> 
> I don't understand this change, sorry. Are you referring to the fact
> that the check is len < sizeof(*th) instead of len != sizeof(*th)?
> 
> Your commit message makes me think that the helpers will access data
> in the extension headers, which isn't true as far as I can tell.

Yes, they will. See bpf_tcp_gen_syncookie -> tcp_v4_get_syncookie -> 
tcp_get_syncookie_mss -> tcp_parse_mss_option, which iterates over the 
TCP options ("extensions" wasn't the best word I used here). Moreover, 
bpf_tcp_gen_syncookie even checks that th_len == th->doff * 4.

Although bpf_tcp_check_syncookie doesn't need the TCP options and 
doesn't enforce them to be passed, it's still allowed.

> That
> would be a problem in fact, since it could be used to read memory that
> the verifier hasn't deemed safe.

It's safe, because bpf_tcp_gen_syncookie reads up to th_len, which is 
ARG_CONST_SIZE for the TCP header.

Re: [PATCH bpf v2 2/4] bpf: Support dual-stack sockets in bpf_tcp_check_syncookie

[Maxim Mikityanskiy]

On 2022-01-26 11:49, Lorenz Bauer wrote:
> On Mon, 24 Jan 2022 at 15:13, Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
>>
>> bpf_tcp_gen_syncookie looks at the IP version in the IP header and
>> validates the address family of the socket. It supports IPv4 packets in
>> AF_INET6 dual-stack sockets.
>>
>> On the other hand, bpf_tcp_check_syncookie looks only at the address
>> family of the socket, ignoring the real IP version in headers, and
>> validates only the packet size. This implementation has some drawbacks:
>>
>> 1. Packets are not validated properly, allowing a BPF program to trick
>>     bpf_tcp_check_syncookie into handling an IPv6 packet on an IPv4
>>     socket.
>>
>> 2. Dual-stack sockets fail the checks on IPv4 packets. IPv4 clients end
>>     up receiving a SYNACK with the cookie, but the following ACK gets
>>     dropped.
>>
>> This patch fixes these issues by changing the checks in
>> bpf_tcp_check_syncookie to match the ones in bpf_tcp_gen_syncookie. IP
>> version from the header is taken into account, and it is validated
>> properly with address family.
>>
>> Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie")
>> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
>> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
>> ---
>>   net/core/filter.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 05efa691b796..780e635fb52a 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -6774,24 +6774,33 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
>>          if (!th->ack || th->rst || th->syn)
>>                  return -ENOENT;
>>
>> +       if (unlikely(iph_len < sizeof(struct iphdr)))
>> +               return -EINVAL;
>> +
>>          if (tcp_synq_no_recent_overflow(sk))
>>                  return -ENOENT;
>>
>>          cookie = ntohl(th->ack_seq) - 1;
>>
>> -       switch (sk->sk_family) {
>> -       case AF_INET:
>> -               if (unlikely(iph_len < sizeof(struct iphdr)))
>> +       /* Both struct iphdr and struct ipv6hdr have the version field at the
>> +        * same offset so we can cast to the shorter header (struct iphdr).
>> +        */
>> +       switch (((struct iphdr *)iph)->version) {
>> +       case 4:
>> +               if (sk->sk_family == AF_INET6 && ipv6_only_sock(sk))
>>                          return -EINVAL;
> 
> Wouldn't this allow an arbitrary value for sk->sk_family, since there
> is no further check that sk_family is AF_INET?

It relies on the assumption that sk_family is either AF_INET or 
AF_INET6, when sk_protocol is IPPROTO_TCP (checked above). The same 
assumption is used in bpf_tcp_gen_syncookie. Do you think there are 
cases when it doesn't hold, and we must verify sk_family? If yes, then 
bpf_tcp_gen_syncookie should also be fixed.

Re: [PATCH bpf v2 3/4] bpf: Use EOPNOTSUPP in bpf_tcp_check_syncookie

[John Fastabend]

Maxim Mikityanskiy wrote:
> On 2022-01-25 09:06, John Fastabend wrote:
> > Maxim Mikityanskiy wrote:
> >> When CONFIG_SYN_COOKIES is off, bpf_tcp_check_syncookie returns
> >> ENOTSUPP. It's a non-standard and deprecated code. The related function
> >> bpf_tcp_gen_syncookie and most of the other functions use EOPNOTSUPP if
> >> some feature is not available. This patch changes ENOTSUPP to EOPNOTSUPP
> >> in bpf_tcp_check_syncookie.
> >>
> >> Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie")
> >> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> >> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
> > 
> > This came up in another thread? Or was it the same and we lost the context
> > in the commit msg. Either way I don't think we should start one-off
> > changing these user facing error codes. Its not the only spot we do this
> > and its been this way for sometime.
> > 
> > Is it causing a real problem?
> 
> I'm not aware of anyone complaining about it. It's just a cleanup to use 
> the proper error code, since ENOTSUPP is a non-standard one (used in 
> NFS?), for example, strerror() returns "Unknown error 524" instead of 
> "Operation not supported".
> 
> Source: Documentation/dev-tools/checkpatch.rst:

iirc we didn't change the other ones so I see no reason to change this. Its
not great, but anything using it has already figured it out and it is
user facing.

Re: [PATCH bpf v2 4/4] bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie

[Lorenz Bauer]

On Mon, 31 Jan 2022 at 13:38, Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
>
> On 2022-01-26 11:45, Lorenz Bauer wrote:
> > On Mon, 24 Jan 2022 at 15:13, Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
> >>
> >> bpf_tcp_gen_syncookie and bpf_tcp_check_syncookie expect the full length
> >> of the TCP header (with all extensions). Fix the documentation that says
> >> it should be sizeof(struct tcphdr).
> >
> > I don't understand this change, sorry. Are you referring to the fact
> > that the check is len < sizeof(*th) instead of len != sizeof(*th)?
> >
> > Your commit message makes me think that the helpers will access data
> > in the extension headers, which isn't true as far as I can tell.
>
> Yes, they will. See bpf_tcp_gen_syncookie -> tcp_v4_get_syncookie ->
> tcp_get_syncookie_mss -> tcp_parse_mss_option, which iterates over the
> TCP options ("extensions" wasn't the best word I used here). Moreover,
> bpf_tcp_gen_syncookie even checks that th_len == th->doff * 4.
>
> Although bpf_tcp_check_syncookie doesn't need the TCP options and
> doesn't enforce them to be passed, it's still allowed.

Sorry, I was only looking at bpf_tcp_check_syncookie indeed.
Unfortunate, it would be better if that function had a th->doff check
as well. :(

Acked-by: Lorenz Bauer <lmb@cloudflare.com>

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com