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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAADnVQLtKtrH-UhaJdn+5d+qObcuQ8TEuVDbpqx2Az=dN1DwWw@mail.gmail.com>
Date:   Fri, 21 Apr 2023 09:09:06 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Florian Westphal <fw@...len.de>
Cc:     bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        netfilter-devel <netfilter-devel@...r.kernel.org>,
        Daniel Xu <dxu@...uu.xyz>, Quentin Deslandes <qde@...cy.de>
Subject: Re: [PATCH bpf-next v4 7/7] selftests/bpf: add missing netfilter
 return value and ctx access tests

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