netdev - Re: Query on reads being flagged as direct writes...

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]

Date:   Fri, 12 Aug 2022 09:58:15 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Maciej Żenczykowski <zenczykowski@...il.com>
Cc:     Lina Wang <lina.wang@...iatek.com>,
        Linux NetDev <netdev@...r.kernel.org>,
        BPF Mailing List <bpf@...r.kernel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Thomas Graf <tgraf@...g.ch>,
        Alexei Starovoitov <ast@...nel.org>
Subject: Re: Query on reads being flagged as direct writes...

On Fri, Aug 12, 2022 at 5:06 AM Maciej Żenczykowski
<zenczykowski@...il.com> wrote:
>
> From kernel/bpf/verifier.c with some simplifications (removed some of
> the cases to make this shorter):
>
> static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
> const struct bpf_call_arg_meta *meta, enum bpf_access_type t)
> {
>   enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
>   switch (prog_type) {
>     /* Program types only with direct read access go here! */
>     case BPF_PROG_TYPE_CGROUP_SKB: (and some others)
>       if (t == BPF_WRITE) return false;
>       fallthrough;
>     /* Program types with direct read + write access go here! */
>     case BPF_PROG_TYPE_SCHED_CLS: (and some others)
>       if (meta) return meta->pkt_access;
>       env->seen_direct_write = true;
>       return true;
>     case BPF_PROG_TYPE_CGROUP_SOCKOPT:
>       if (t == BPF_WRITE) env->seen_direct_write = true;
>       return true;
>   }
> }
>
> why does the above set env->seen_direct_write to true even when t !=
> BPF_WRITE, even for programs that only allow (per the comment) direct
> read access.
>
> Is this working correctly?  Is there some gotcha this is papering over?
>
> Should 'env->seen_direct_write = true; return true;' be changed into
> 'fallthrough' so that write is only set if t == BPF_WRITE?
>
> This matters because 'env->seen_direct_write = true' then triggers an
> unconditional unclone in the bpf prologue, which I'd like to avoid
> unless I actually need to modify the packet (with
> bpf_skb_store_bytes)...
>
> may_access_direct_pkt_data() has two call sites, in one it only gets
> called with BPF_WRITE so it's ok, but the other one is in
> check_func_arg():
>
> if (type_is_pkt_pointer(type) && !may_access_direct_pkt_data(env,
> meta, BPF_READ)) { verbose(env, "helper access to the packet is not
> allowed\n"); return -EACCES; }
>
> and I'm not really following what this does, but it seems like bpf
> helper read access to the packet triggers unclone?

There seems to be a set of helpers (pkt_access=true) which accept
direct packet pointers and are known to be doing only reads of the skb
data (safe without clone).
You seem to be hitting the case where you're passing that packet
pointer to one of the "unsafe" (pkt_acces=false) helpers which
triggers that seen_direct_write=true condition.
So it seems like it's by design? Which helper are you calling? Maybe
that one should also have pkt_access=true?

Tangential: I wish there was an explicit BPF_F_MAY_ATTEMPT_TO_CLONE
flag that gates this auto-clone. I think at some point we also
accidentally hit it :-(

> (side note: all packets ingressing from the rndis gadget driver are
> clones due to how it deals with usb packet deaggregation [not to be
> mistaken with lro/tso])
>
> Confused...