Re: [PATCH bpf-next v4 7/7] selftests/bpf: add missing netfilter return value and ctx access tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Apr 21, 2023 at 8:52 AM Florian Westphal <fw@xxxxxxxxx> wrote:
>
> Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> 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.




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux