netdev - Re: [RFC bpf-next 0/7] net: flow_dissector: trigger BPF hook when called from eth_get

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

Date:   Wed, 13 Feb 2019 22:38:50 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Stanislav Fomichev <sdf@...ichev.me>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Stanislav Fomichev <sdf@...gle.com>,
        Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        simon.horman@...ronome.com, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [RFC bpf-next 0/7] net: flow_dissector: trigger BPF hook when
 called from eth_get_headlen

On Wed, Feb 13, 2019 at 09:57:25PM -0800, Stanislav Fomichev wrote:
> 
> > That 'stuck with __sk_buff' is what bothers me.
> I might have use the wrong word here. I don't think there is another
> option to be honest. Using __sk_buff makes flow dissector programs work
> with fragmented packets;

good point. indeed real skb is essential.

> > It's an indication that api wasn't thought through if first thing
> > it needs is this fake skb hack.
> > If bpf_flow.c is a realistic example of such flow dissector prog
> > it means that real skb fields are accessed.
> > In particular skb->vlan_proto, skb->protocol.
> I do manually set skb->protocol to eth->h_proto in my proposal. This is later
> correctly handled by bpf_flow.c: parse_eth_proto() is called on skb->protocol
> and we correctly handle bpf_htons(ETH_P_8021Q) there. So existing
> bpf_flow.c works as expected.
...
> The goal of this patch series was to essentially make this skb/no-skb
> context transparent to the bpf_flow.c (i.e. no changes from the user
> flow programs). Adding another flow dissector for eth_get_headlen case
> also seems as a no go.

The problem with this thinking is assumption that bpf_flow.c is the only program.
Since ctx of flow_dissector prog type is 'struct __sk_buff'
all fields should be valid or the verifier has to reject access
to fields that were not set.
You cannot "manually set skb->protocol to eth->h_proto" in fake skb
and ignore the rest.