netdev - 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 12 Aug 2022 05:06:06 -0700
From:   Maciej Żenczykowski <zenczykowski@...il.com>
To:     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>,
        Stanislav Fomichev <sdf@...gle.com>,
        Thomas Graf <tgraf@...g.ch>,
        Alexei Starovoitov <ast@...nel.org>
Subject: Query on reads being flagged as direct writes...

>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?

(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...