netdev - Re: [PATCH net-next v4 1/3] net: flow_dissector: extend bpf flow dissector support with vnet hdr

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>] [day] [month] [year] [list]

Date:   Wed, 9 Jun 2021 11:24:50 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Tanner Love <tannerlove@...gle.com>
CC:     Tanner Love <tannerlove.kernel@...il.com>,
        <netdev@...r.kernel.org>, <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Petar Penkov <ppenkov@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "Michael S . Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        Stanislav Fomichev <sdf@...gle.com>
Subject: Re: [PATCH net-next v4 1/3] net: flow_dissector: extend bpf flow
 dissector support with vnet hdr

On Wed, Jun 09, 2021 at 10:12:36AM -0700, Tanner Love wrote:
[ ... ]

> > > @@ -4218,6 +4243,23 @@ static int check_mem_access(struct
> > bpf_verifier_env *env, int insn_idx, u32 regn
> > >               }
> > >
> > >               err = check_flow_keys_access(env, off, size);
> > > +             if (!err && t == BPF_READ && value_regno >= 0) {
> > > +                     if (off == offsetof(struct bpf_flow_keys, vhdr)) {
> > > +                             regs[value_regno].type =
> > PTR_TO_VNET_HDR_OR_NULL;
> > check_flow_keys_access() needs some modifications
> >
> > 1. What if t == BPF_WRITE?  I think "keys->vhdr = 0xdead" has to be
> > rejected.
> >
> > 2. It needs to check loading keys->vhdr is in sizeof(__u64) like other
> >    pointer loading does.  Take a look at the flow_keys case in
> >    flow_dissector_is_valid_access().
> >
> > It also needs to convert the pointer loading like how
> > flow_dissector_convert_ctx_access() does on flow_keys.
> >
> 
> I understand 1 and 2, and I agree. I will make the changes in the
> next version. Thanks. But I do not understand your comment "It
> also needs to convert the pointer loading like how
> flow_dissector_convert_ctx_access() does on flow_keys."
> Convert it to what? The pointer to struct virtio_net_hdr is in struct
> bpf_flow_keys, not struct bpf_flow_dissector (the kernel context).
> Could you please elaborate? Thank you!
Ah, right. There is no kernel counter part for bpf_flow_keys.
Please ignore the "convert the pointer loading" comment.

> 
> >
> > A high level design question.  "struct virtio_net_hdr" is in uapi and
> > there is no need to do convert_ctx.  I think using PTR_TO_BTF_ID_OR_NULL
> > will be easier here and the new PTR_TO_VNET_HDR* related changes will go
> > away.
> >
> > The "else if (reg->type == PTR_TO_CTX)" case earlier could be a good
> > example.
> >
> > To get the btf_id for "struct virtio_net_hdr", take a look at
> > the BTF_ID_LIST_SINGLE() usage in filter.c
> >
> 
> Thanks for the suggestion. Still ruminating on this, but figured I'd send my
> above question in the meantime.
btf_id points to a BTF debuginfo that describes how a kernel struct looks like.

PTR_TO_BTF_ID(_NOT_NULL) means a reg is a pointer to a kernel struct described
by a BTF (so the btf_id).  With the BTF, the access is checked commonly
in btf_struct_access() for any kernel struct.

It needs the BPF_PROBE_MEM support in the JIT which is currently in
x86/arm64/s390.