| Message ID | 20220124151146.376446-5-maximmi@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Bugfixes for syncookie BPF helpers | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf | success | VM_Test |
| bpf/vmtest-bpf-PR | success | PR summary |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 1794 this patch: 1794 |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: joe@cilium.io |
| netdev/build_clang | success | Errors and warnings before: 211 this patch: 211 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1811 this patch: 1811 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 36 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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>
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.
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.
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>
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,
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(-)