netdev - Re: [PATCH v2 bpf-next 4/9] bpf: add bpf helper bpf_skb_ecn_set

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 for Android: free password hash cracker in your pocket

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

Message-ID: <b6b1ceb4-8a69-a92d-ea20-1d3f228ccd30@gmail.com>
Date:   Mon, 25 Feb 2019 08:52:35 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>, Martin Lau <kafai@...com>
Cc:     Lawrence Brakmo <brakmo@...com>, netdev <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 4/9] bpf: add bpf helper bpf_skb_ecn_set_ce



On 02/25/2019 02:10 AM, Daniel Borkmann wrote:

> My understanding is that before doing any writes into skb, we should make
> sure the data area is private to us (and offset in linear data). In tc BPF
> (ingress, egress) we use bpf_try_make_writable() helper for this, others
> like act_{pedit,skbmod} or ovs have similar logic before writing into skb,
> note that in all these cases it's mostly about generic writes, so location
> could also be L4, for example.
> 
> Difference of above helper compared to net/sched/sch_*.c instances could
> be that it's i) for the qdisc case it's only on egress INET_ECN_set_ce()
> and that there may be a convention that qdiscs specifically may mangle
> it whereas the helper could be called on ingress and egress and confuse
> other subsystems since they won't see original or race by seeing partially
> updated (invalid) packet.
> 
> Eric, have a chance to clarify? Perhaps then would make sense to disallow
> the helper in cgroup ingress path.

Good observations Daniel, thanks for bringing this up.

skb_ensure_writable() seems a big hammer for the case we change some bits in IP header.

TCP cloned packets certainly can have their headers mangled, so maybe
we need to use something using skb_header_cloned() instead of skb_cloned()