Bugfixes for syncookie BPF helpers
Subsystems (4)
v2 cover Bugfixes for syncookie BPF helpers
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/[email protected]/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(-)
Signature · 1 line
2.30.2
PATCH v2 1/4 bpf: Use ipv6_only_sock in bpf_tcp_gen_syncookie
Instead of querying the sk_ipv6only field directly, use the dedicated ipv6_only_sock helper.
--- net/core/filter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/core/filter.c +1 -1
@@ -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);
Signature · 1 line
2.30.2
RE: [PATCH bpf v2 1/4] bpf: Use ipv6_only_sock in bpf_tcp_gen_syncookie
Quoted 7 lines · Maxim Mikityanskiy
Instead of querying the
sk_ipv6onlyfield directly, use the dedicatedipv6_only_sockhelper.Fixes 70d66244317e ("bpf: add bpf_tcp_gen_syncookie helper")Signed-off-by Maxim Mikityanskiy <[email protected]>Reviewed-by Tariq Toukan <[email protected]>---
Not really a fix, but LGTM.
Re: [PATCH bpf v2 1/4] bpf: Use ipv6_only_sock in bpf_tcp_gen_syncookie
In reply to Maxim MikityanskiyQuoted 7 lines · Maxim Mikityanskiy
Instead of querying the
sk_ipv6onlyfield directly, use the dedicatedipv6_only_sockhelper.Fixes 70d66244317e ("bpf: add bpf_tcp_gen_syncookie helper")Signed-off-by Maxim Mikityanskiy <[email protected]>Reviewed-by Tariq Toukan <[email protected]>
Signature · 4 lines
Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
Petar Penkov
Acked-byRe: [PATCH bpf v2 1/4] bpf: Use ipv6_only_sock in bpf_tcp_gen_syncookie
Quoted 11 lines · Lorenz Bauer
Quoted 7 lines · Maxim Mikityanskiy
Instead of querying the
sk_ipv6onlyfield directly, use the dedicatedipv6_only_sockhelper.Fixes 70d66244317e ("bpf: add bpf_tcp_gen_syncookie helper")Signed-off-by Maxim Mikityanskiy <[email protected]>Reviewed-by Tariq Toukan <[email protected]>Acked-by Lorenz Bauer <[email protected]>
PATCH v2 2/4 bpf: Support dual-stack sockets in bpf_tcp_check_syncookie
In reply to Maxim Mikityanskiybpf_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.
--- net/core/filter.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
net/core/filter.c +13 -4
@@ -6774,24 +6774,33 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_lenif (!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 */
Signature · 1 line
2.30.2
RE: [PATCH bpf v2 2/4] bpf: Support dual-stack sockets in bpf_tcp_check_syncookie
Quoted 11 lines · Maxim Mikityanskiy
bpf_tcp_gen_syncookielooks at the IP version in the IP header and validates the address family of the socket. It supports IPv4 packets inAF_INET6dual-stack sockets.On the other hand,
bpf_tcp_check_syncookielooks 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_syncookieinto 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.
Quoted 4 lines
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.
Quoted 5 lines
This patch fixes these issues by changing the checks in
bpf_tcp_check_syncookieto match the ones inbpf_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.
Quoted 6 lines
Fixes 399040847084 ("bpf: add helper to check for a valid SYN cookie")Signed-off-by Maxim Mikityanskiy <[email protected]>Reviewed-by Tariq Toukan <[email protected]>--- net/core/filter.c | 17 +++++++++++++----
Re: [PATCH bpf v2 2/4] bpf: Support dual-stack sockets in bpf_tcp_check_syncookie
In reply to Maxim MikityanskiyQuoted 45 lines · Maxim Mikityanskiy
bpf_tcp_gen_syncookielooks at the IP version in the IP header and validates the address family of the socket. It supports IPv4 packets inAF_INET6dual-stack sockets.On the other hand,
bpf_tcp_check_syncookielooks 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_syncookieinto 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_syncookieto match the ones inbpf_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 <[email protected]>Reviewed-by Tariq Toukan <[email protected]>--- net/core/filter.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)net/core/filter.c +3
-0@@ -6774,24 +6774,33 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_lenif (!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 ;
net/core/filter.c BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len · line 6791
Quoted 10 lines
net/core/filter.c +6
-3-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?
Signature · 4 lines
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
Quoted 46 lines · Lorenz Bauer
Quoted 45 lines · Maxim Mikityanskiy
bpf_tcp_gen_syncookielooks at the IP version in the IP header and validates the address family of the socket. It supports IPv4 packets inAF_INET6dual-stack sockets.On the other hand,
bpf_tcp_check_syncookielooks 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_syncookieinto 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_syncookieto match the ones inbpf_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 <[email protected]>Reviewed-by Tariq Toukan <[email protected]>--- net/core/filter.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)net/core/filter.c +3
-0@@ -6774,24 +6774,33 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_lenif (!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 ;
Quoted 13 lines
net/core/filter.c BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len · line 6791
Quoted 10 lines
net/core/filter.c +6
-3-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 thatsk_familyisAF_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.
PATCH v2 3/4 bpf: Use EOPNOTSUPP in bpf_tcp_check_syncookie
In reply to Maxim MikityanskiyWhen 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.
--- net/core/filter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/core/filter.c +1 -1
@@ -6814,7 +6814,7 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_lenreturn -ENOENT ;#else -return -ENOTSUPP ; +return -EOPNOTSUPP ;#endif }
Signature · 1 line
2.30.2
John Fastabend
reviewerRE: [PATCH bpf v2 3/4] bpf: Use EOPNOTSUPP in bpf_tcp_check_syncookie
Quoted 9 lines · Maxim Mikityanskiy
When
CONFIG_SYN_COOKIESis off,bpf_tcp_check_syncookiereturns ENOTSUPP. It's a non-standard and deprecated code. The related functionbpf_tcp_gen_syncookieand most of the other functions use EOPNOTSUPP if some feature is not available. This patch changes ENOTSUPP to EOPNOTSUPP inbpf_tcp_check_syncookie.Fixes 399040847084 ("bpf: add helper to check for a valid SYN cookie")Signed-off-by Maxim Mikityanskiy <[email protected]>Reviewed-by Tariq Toukan <[email protected]>
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?
Quoted 20 lines
--- net/core/filter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/core/filter.c +1
-1@@ -6814,7 +6814,7 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_lenreturn -ENOENT ;#else -return -ENOTSUPP ; +return -EOPNOTSUPP ;#endif }Signature · 1 line
2.30.2
Re: [PATCH bpf v2 3/4] bpf: Use EOPNOTSUPP in bpf_tcp_check_syncookie
Quoted 11 lines · John Fastabend
Quoted 9 lines · Maxim Mikityanskiy
When
CONFIG_SYN_COOKIESis off,bpf_tcp_check_syncookiereturns ENOTSUPP. It's a non-standard and deprecated code. The related functionbpf_tcp_gen_syncookieand most of the other functions use EOPNOTSUPP if some feature is not available. This patch changes ENOTSUPP to EOPNOTSUPP inbpf_tcp_check_syncookie.Fixes 399040847084 ("bpf: add helper to check for a valid SYN cookie")Signed-off-by Maxim Mikityanskiy <[email protected]>Reviewed-by Tariq Toukan <[email protected]>
Quoted 6 lines
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:
Quoted 25 lines
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/[email protected]/
Quoted 20 lines
--- net/core/filter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/core/filter.c +1
-1@@ -6814,7 +6814,7 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_lenreturn -ENOENT ;#else -return -ENOTSUPP ; +return -EOPNOTSUPP ;#endif }Signature · 1 line
2.30.2
John Fastabend
reviewerRe: [PATCH bpf v2 3/4] bpf: Use EOPNOTSUPP in bpf_tcp_check_syncookie
Quoted 17 lines · Maxim Mikityanskiy
Quoted 16 lines · John Fastabend
Quoted 9 lines · Maxim Mikityanskiy
When
CONFIG_SYN_COOKIESis off,bpf_tcp_check_syncookiereturns ENOTSUPP. It's a non-standard and deprecated code. The related functionbpf_tcp_gen_syncookieand most of the other functions use EOPNOTSUPP if some feature is not available. This patch changes ENOTSUPP to EOPNOTSUPP inbpf_tcp_check_syncookie.Fixes 399040847084 ("bpf: add helper to check for a valid SYN cookie")Signed-off-by Maxim Mikityanskiy <[email protected]>Reviewed-by Tariq Toukan <[email protected]>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.
Quoted 8 lines
Quoted 1 line
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.
PATCH v2 4/4 bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie
In reply to Maxim Mikityanskiybpf_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).
--- include/uapi/linux/bpf.h | 6 ++++-- tools/include/uapi/linux/bpf.h | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-)
include/uapi/linux/bpf.h +4 -2
@@ -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,
tools/include/uapi/linux/bpf.h +4 -2
@@ -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,
Signature · 1 line
2.30.2
RE: [PATCH bpf v2 4/4] bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie
Quoted 8 lines · Maxim Mikityanskiy
bpf_tcp_gen_syncookieandbpf_tcp_check_syncookieexpect 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 <[email protected]>Reviewed-by Tariq Toukan <[email protected]>
Not sure I would push doc fixes at bpf tree, would be fine to go through bpf-next. But change looks good.
Re: [PATCH bpf v2 4/4] bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie
In reply to Maxim MikityanskiyQuoted 4 lines · Maxim Mikityanskiy
bpf_tcp_gen_syncookieandbpf_tcp_check_syncookieexpect 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.
Signature · 4 lines
Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
Re: [PATCH bpf v2 4/4] bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie
Quoted 11 lines · Lorenz Bauer
Quoted 4 lines · Maxim Mikityanskiy
bpf_tcp_gen_syncookieandbpf_tcp_check_syncookieexpect 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.
Quoted 3 lines
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.
Lorenz Bauer
Acked-byRe: [PATCH bpf v2 4/4] bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie
Quoted 14 lines · Maxim Mikityanskiy
Quoted 11 lines · Lorenz Bauer
Quoted 4 lines · Maxim Mikityanskiy
bpf_tcp_gen_syncookieandbpf_tcp_check_syncookieexpect 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.
Quoted 7 lines
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_syncookieeven checks thatth_len==th->doff* 4.Although
bpf_tcp_check_syncookiedoesn'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. :(
Signature · 4 lines
Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
John Fastabend
reviewerQuoted 3 lines · 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.
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