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
| ||
|
Message-ID: <20190225203337.GA32115@mini-arch> Date: Mon, 25 Feb 2019 12:33:37 -0800 From: Stanislav Fomichev <sdf@...ichev.me> To: Alexei Starovoitov <alexei.starovoitov@...il.com> 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 02/14, Stanislav Fomichev wrote: > On 02/13, Alexei Starovoitov wrote: > > 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. > I agree, it's a bad assumption, but it is sort of a reference implementation, > I don't expect other users to do something wildly different. Hopefully :-) > > > 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. > Ugh, I did expect that we only allow a minimal set of __sk_buff fields > to be allowed from the flow dissector program type, but that's not the > case. We explicitly prohibit access only to > family/ips/ports/tc_classid/tstamp/wire_len, everything else is readable :-/ > Any idea why? > Stuff like ingress_ifindex/ifindex/hash/mark/queue_mapping, does flow dissector > programs really need to know that? > > For the most part, using zero-initialized fake skb looks fine, except: > * infindex, where we do skb->dev->ifndex (skb->dev is NULL) > * gso_segs, where we do skb_shinfo(skb)->gso_segs (we are missing > shinfo) > > So there is indeed a couple of problems. > > How do you feel about tightening down the access to sk_buff fields from > the flow dissector program type? That is an API change, but I don't see why > existing users should use those fields. Let's allow access only to > len/data/data_end, protocol, vlan_{present,tci,proto}, cb, flow_keys, > that should be enough to dissect the packet (I also looked at C-based > implementation, it doesn't use anything besides that). > We can always rollback if somebody complains about. To revive the conversation, here is what I was thinking about (whitelist the skb fields, not blacklist them): --- a/net/core/filter.c +++ b/net/core/filter.c @@ -6591,11 +6591,14 @@ static bool flow_dissector_is_valid_access(int off, int size, case bpf_ctx_range_ptr(struct __sk_buff, flow_keys): info->reg_type = PTR_TO_FLOW_KEYS; break; - case bpf_ctx_range(struct __sk_buff, tc_classid): - case bpf_ctx_range(struct __sk_buff, data_meta): - case bpf_ctx_range_till(struct __sk_buff, family, local_port): - case bpf_ctx_range(struct __sk_buff, tstamp): - case bpf_ctx_range(struct __sk_buff, wire_len): + case bpf_ctx_range(struct __sk_buff, len): + case bpf_ctx_range(struct __sk_buff, protocol): + case bpf_ctx_range(struct __sk_buff, vlan_present): + case bpf_ctx_range(struct __sk_buff, vlan_tci): + case bpf_ctx_range(struct __sk_buff, vlan_proto): + case bpf_ctx_range_till(struct __sk_buff, cb[0], cb[4]): + break; + default: return false; } What do you think?
Powered by blists - more mailing lists