| Message ID | 20230420124455.31099-8-fw@strlen.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: add netfilter program type | expand |
On Thu, Apr 20, 2023 at 02:44:55PM +0200, Florian Westphal wrote: > + > +SEC("netfilter") > +__description("netfilter valid context access") > +__success __failure_unpriv > +__retval(1) > +__naked void with_invalid_ctx_access_test5(void) > +{ > + asm volatile (" \ > + r2 = *(u64*)(r1 + %[__bpf_nf_ctx_state]); \ > + r1 = *(u64*)(r1 + %[__bpf_nf_ctx_skb]); \ > + r0 = 1; \ > + exit; \ > +" : > + : __imm_const(__bpf_nf_ctx_state, offsetof(struct bpf_nf_ctx, state)), > + __imm_const(__bpf_nf_ctx_skb, offsetof(struct bpf_nf_ctx, skb)) > + : __clobber_all); Could you write this one in C instead? Also check that skb and state are dereferenceable after that. Since they should be seen as trusted ptr_to_btf_id skb->len and state->sk should work. You cannot craft this test case in asm, since it needs CO-RE. Also see that BPF CI is not happy: https://github.com/kernel-patches/bpf/actions/runs/4757642030/jobs/8455500277 Error: #112 libbpf_probe_prog_types Error: #112/32 libbpf_probe_prog_types/BPF_PROG_TYPE_NETFILTER Error: #113 libbpf_str Error: #113/4 libbpf_str/bpf_prog_type_str
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > On Thu, Apr 20, 2023 at 02:44:55PM +0200, Florian Westphal wrote: > > + > > +SEC("netfilter") > > +__description("netfilter valid context access") > > +__success __failure_unpriv > > +__retval(1) > > +__naked void with_invalid_ctx_access_test5(void) > > +{ > > + asm volatile (" \ > > + r2 = *(u64*)(r1 + %[__bpf_nf_ctx_state]); \ > > + r1 = *(u64*)(r1 + %[__bpf_nf_ctx_skb]); \ > > + r0 = 1; \ > > + exit; \ > > +" : > > + : __imm_const(__bpf_nf_ctx_state, offsetof(struct bpf_nf_ctx, state)), > > + __imm_const(__bpf_nf_ctx_skb, offsetof(struct bpf_nf_ctx, skb)) > > + : __clobber_all); > > Could you write this one in C instead? > > Also check that skb and state are dereferenceable after that. My bad. Added this and that: SEC("netfilter") __description("netfilter valid context read and invalid write") __failure __msg("only read is supported") int with_invalid_ctx_access_test5(struct bpf_nf_ctx *ctx) { struct nf_hook_state *state = (void *)ctx->state; state->sk = NULL; return 1; } SEC("netfilter") __description("netfilter test prog with skb and state read access") __success __failure_unpriv __retval(0) int with_valid_ctx_access_test6(struct bpf_nf_ctx *ctx) { const struct nf_hook_state *state = ctx->state; struct sk_buff *skb = ctx->skb; const struct iphdr *iph; const struct tcphdr *th; u8 buffer_iph[20] = {}; u8 buffer_th[40] = {}; struct bpf_dynptr ptr; uint8_t ihl; if (skb->len <= 20 || bpf_dynptr_from_skb(skb, 0, &ptr)) return 1; iph = bpf_dynptr_slice(&ptr, 0, buffer_iph, sizeof(buffer_iph)); if (!iph) return 1; if (state->pf != 2) return 1; ihl = iph->ihl << 2; th = bpf_dynptr_slice(&ptr, ihl, buffer_th, sizeof(buffer_th)); if (!th) return 1; return th->dest == bpf_htons(22) ? 1 : 0; } "Worksforme". Is there anything else thats missing? If not I'll send v5 on Monday. > Since they should be seen as trusted ptr_to_btf_id skb->len and state->sk should work. > You cannot craft this test case in asm, since it needs CO-RE. > > Also see that BPF CI is not happy: > https://github.com/kernel-patches/bpf/actions/runs/4757642030/jobs/8455500277 > Error: #112 libbpf_probe_prog_types > Error: #112/32 libbpf_probe_prog_types/BPF_PROG_TYPE_NETFILTER > Error: #113 libbpf_str > Error: #113/4 libbpf_str/bpf_prog_type_str prog_type_name[] lacks "netfilter" entry, and a missing 'case PROG_NETFILTER', v5 should pass this now.
On Fri, Apr 21, 2023 at 8:52 AM Florian Westphal <fw@strlen.de> wrote: > > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Apr 20, 2023 at 02:44:55PM +0200, Florian Westphal wrote: > > > + > > > +SEC("netfilter") > > > +__description("netfilter valid context access") > > > +__success __failure_unpriv > > > +__retval(1) > > > +__naked void with_invalid_ctx_access_test5(void) > > > +{ > > > + asm volatile (" \ > > > + r2 = *(u64*)(r1 + %[__bpf_nf_ctx_state]); \ > > > + r1 = *(u64*)(r1 + %[__bpf_nf_ctx_skb]); \ > > > + r0 = 1; \ > > > + exit; \ > > > +" : > > > + : __imm_const(__bpf_nf_ctx_state, offsetof(struct bpf_nf_ctx, state)), > > > + __imm_const(__bpf_nf_ctx_skb, offsetof(struct bpf_nf_ctx, skb)) > > > + : __clobber_all); > > > > Could you write this one in C instead? > > > > Also check that skb and state are dereferenceable after that. > > My bad. Added this and that: > > SEC("netfilter") > __description("netfilter valid context read and invalid write") > __failure __msg("only read is supported") > int with_invalid_ctx_access_test5(struct bpf_nf_ctx *ctx) > { > struct nf_hook_state *state = (void *)ctx->state; > > state->sk = NULL; > return 1; > } > > SEC("netfilter") > __description("netfilter test prog with skb and state read access") > __success __failure_unpriv > __retval(0) > int with_valid_ctx_access_test6(struct bpf_nf_ctx *ctx) > { > const struct nf_hook_state *state = ctx->state; > struct sk_buff *skb = ctx->skb; > const struct iphdr *iph; > const struct tcphdr *th; > u8 buffer_iph[20] = {}; > u8 buffer_th[40] = {}; > struct bpf_dynptr ptr; > uint8_t ihl; > > if (skb->len <= 20 || bpf_dynptr_from_skb(skb, 0, &ptr)) > return 1; Use NF_ACCEPT instead of 1 ? Sadly it's not an enum yet, so it's not in vmlinux.h The prog would need to manually #define it. > > iph = bpf_dynptr_slice(&ptr, 0, buffer_iph, sizeof(buffer_iph)); > if (!iph) > return 1; > > if (state->pf != 2) > return 1; > > ihl = iph->ihl << 2; > th = bpf_dynptr_slice(&ptr, ihl, buffer_th, sizeof(buffer_th)); > if (!th) > return 1; > > return th->dest == bpf_htons(22) ? 1 : 0; > } Perfect. That's what I wanted to see. Without above example it's hard for people to see how ctx->skb can be accessed to parse the packet. > "Worksforme". Is there anything else thats missing? > If not I'll send v5 on Monday. ship it any time. Don't delay.
diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c index 25bc8958dbfe..2b4ded63ce9f 100644 --- a/tools/testing/selftests/bpf/prog_tests/verifier.c +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c @@ -29,6 +29,8 @@ #include "verifier_map_ret_val.skel.h" #include "verifier_masking.skel.h" #include "verifier_meta_access.skel.h" +#include "verifier_netfilter_ctx.skel.h" +#include "verifier_netfilter_retcode.skel.h" #include "verifier_raw_stack.skel.h" #include "verifier_raw_tp_writable.skel.h" #include "verifier_reg_equal.skel.h" @@ -94,6 +96,8 @@ void test_verifier_map_ptr(void) { RUN(verifier_map_ptr); } void test_verifier_map_ret_val(void) { RUN(verifier_map_ret_val); } void test_verifier_masking(void) { RUN(verifier_masking); } void test_verifier_meta_access(void) { RUN(verifier_meta_access); } +void test_verifier_netfilter_ctx(void) { RUN(verifier_netfilter_ctx); } +void test_verifier_netfilter_retcode(void) { RUN(verifier_netfilter_retcode); } void test_verifier_raw_stack(void) { RUN(verifier_raw_stack); } void test_verifier_raw_tp_writable(void) { RUN(verifier_raw_tp_writable); } void test_verifier_reg_equal(void) { RUN(verifier_reg_equal); } diff --git a/tools/testing/selftests/bpf/progs/verifier_netfilter_ctx.c b/tools/testing/selftests/bpf/progs/verifier_netfilter_ctx.c new file mode 100644 index 000000000000..861b2266179f --- /dev/null +++ b/tools/testing/selftests/bpf/progs/verifier_netfilter_ctx.c @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include "vmlinux.h" + +#include "bpf_misc.h" + +#include <bpf/bpf_tracing.h> +#include <bpf/bpf_helpers.h> + +SEC("netfilter") +__description("netfilter invalid context access, size too short") +__failure __msg("invalid bpf_context access") +__naked void with_invalid_ctx_access_test1(void) +{ + asm volatile (" \ + r2 = *(u8*)(r1 + %[__bpf_nf_ctx_state]); \ + r0 = 0; \ + exit; \ +" : + : __imm_const(__bpf_nf_ctx_state, offsetof(struct bpf_nf_ctx, state)) + : __clobber_all); +} + +SEC("netfilter") +__description("netfilter invalid context access, size too short") +__failure __msg("invalid bpf_context access") +__naked void with_invalid_ctx_access_test2(void) +{ + asm volatile (" \ + r2 = *(u16*)(r1 + %[__bpf_nf_ctx_skb]); \ + r0 = 0; \ + exit; \ +" : + : __imm_const(__bpf_nf_ctx_skb, offsetof(struct bpf_nf_ctx, skb)) + : __clobber_all); +} + +SEC("netfilter") +__description("netfilter invalid context access, past end of ctx") +__failure __msg("invalid bpf_context access") +__naked void with_invalid_ctx_access_test3(void) +{ + asm volatile (" \ + r2 = *(u64*)(r1 + %[__bpf_nf_ctx_size]); \ + r0 = 0; \ + exit; \ +" : + : __imm_const(__bpf_nf_ctx_size, sizeof(struct bpf_nf_ctx)) + : __clobber_all); +} + +SEC("netfilter") +__description("netfilter invalid context, write") +__failure __msg("invalid bpf_context access") +__naked void with_invalid_ctx_access_test4(void) +{ + asm volatile (" \ + r2 = r1; \ + *(u64*)(r2 + 0) = r1; \ + r0 = 1; \ + exit; \ +" : + : __imm_const(__bpf_nf_ctx_skb, offsetof(struct bpf_nf_ctx, skb)) + : __clobber_all); +} + +SEC("netfilter") +__description("netfilter valid context access") +__success __failure_unpriv +__retval(1) +__naked void with_invalid_ctx_access_test5(void) +{ + asm volatile (" \ + r2 = *(u64*)(r1 + %[__bpf_nf_ctx_state]); \ + r1 = *(u64*)(r1 + %[__bpf_nf_ctx_skb]); \ + r0 = 1; \ + exit; \ +" : + : __imm_const(__bpf_nf_ctx_state, offsetof(struct bpf_nf_ctx, state)), + __imm_const(__bpf_nf_ctx_skb, offsetof(struct bpf_nf_ctx, skb)) + : __clobber_all); +} diff --git a/tools/testing/selftests/bpf/progs/verifier_netfilter_retcode.c b/tools/testing/selftests/bpf/progs/verifier_netfilter_retcode.c new file mode 100644 index 000000000000..353ae6da00e1 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/verifier_netfilter_retcode.c @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> +#include "bpf_misc.h" + +SEC("netfilter") +__description("bpf_exit with invalid return code. test1") +__failure __msg("R0 is not a known value") +__naked void with_invalid_return_code_test1(void) +{ + asm volatile (" \ + r0 = *(u64*)(r1 + 0); \ + exit; \ +" ::: __clobber_all); +} + +SEC("netfilter") +__description("bpf_exit with valid return code. test2") +__success +__naked void with_valid_return_code_test2(void) +{ + asm volatile (" \ + r0 = 0; \ + exit; \ +" ::: __clobber_all); +} + +SEC("netfilter") +__description("bpf_exit with valid return code. test3") +__success +__naked void with_valid_return_code_test3(void) +{ + asm volatile (" \ + r0 = 1; \ + exit; \ +" ::: __clobber_all); +} + +SEC("netfilter") +__description("bpf_exit with invalid return code. test4") +__failure __msg("R0 has value (0x2; 0x0)") +__naked void with_invalid_return_code_test4(void) +{ + asm volatile (" \ + r0 = 2; \ + exit; \ +" ::: __clobber_all); +}
Extend prog_tests with two test cases: # ./test_progs --allow=verifier_netfilter_retcode #278/1 verifier_netfilter_retcode/bpf_exit with invalid return code. test1:OK #278/2 verifier_netfilter_retcode/bpf_exit with valid return code. test2:OK #278/3 verifier_netfilter_retcode/bpf_exit with valid return code. test3:OK #278/4 verifier_netfilter_retcode/bpf_exit with invalid return code. test4:OK #278 verifier_netfilter_retcode:OK This checks that only accept and drop (0,1) are permitted. NF_QUEUE could be implemented later if we can guarantee that attachment of such programs can be rejected if they get attached to a pf/hook that doesn't support async reinjection. NF_STOLEN could be implemented via trusted helpers that can guarantee that the skb will eventually be free'd. v4: test case for bpf_nf_ctx access checks, requested by Alexei Starovoitov. # ./test_progs --allow=verifier_netfilter_ctx #280/1 verifier_netfilter_ctx/netfilter invalid context access, size too short:OK #280/2 verifier_netfilter_ctx/netfilter invalid context access, size too short:OK #280/3 verifier_netfilter_ctx/netfilter invalid context access, past end of ctx:OK #280/4 verifier_netfilter_ctx/netfilter invalid context, write:OK #280/5 verifier_netfilter_ctx/netfilter valid context access:OK #280/6 verifier_netfilter_ctx/netfilter valid context access @unpriv:OK #280 verifier_netfilter_ctx:OK Summary: 1/6 PASSED, 0 SKIPPED, 0 FAILED This checks: 1/2: partial reads of ctx->{skb,state} are rejected 3. read access past sizeof(ctx) is rejected 4. write to ctx content, e.g. 'ctx->skb = NULL;' is rejected 5. ctx->skb and ctx->state can be read (valid case), but ... 6. ... same program fails for unpriv (CAP_NET_ADMIN needed). Link: https://lore.kernel.org/bpf/20230419021152.sjq4gttphzzy6b5f@dhcp-172-26-102-232.dhcp.thefacebook.com/ Signed-off-by: Florian Westphal <fw@strlen.de> --- .../selftests/bpf/prog_tests/verifier.c | 4 + .../bpf/progs/verifier_netfilter_ctx.c | 82 +++++++++++++++++++ .../bpf/progs/verifier_netfilter_retcode.c | 49 +++++++++++ 3 files changed, 135 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/verifier_netfilter_ctx.c create mode 100644 tools/testing/selftests/bpf/progs/verifier_netfilter_retcode.c