netdev - Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf

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:   Sun, 28 Jun 2020 13:00:46 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Yonghong Song <yhs@...com>, Jiri Olsa <jolsa@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Song Liu <songliubraving@...com>,
        Martin KaFai Lau <kafai@...com>,
        David Miller <davem@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Wenbo Zhang <ethercflow@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Andrii Nakryiko <andriin@...com>,
        Brendan Gregg <bgregg@...flix.com>,
        Florent Revest <revest@...omium.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving

On Sun, Jun 28, 2020 at 11:50 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Fri, Jun 26, 2020 at 04:29:23PM -0700, Yonghong Song wrote:
> > > > >
> > > > > -int btf_resolve_helper_id(struct bpf_verifier_log *log,
> > > > > -                       const struct bpf_func_proto *fn, int arg)
> > > > > -{
> > > > > -     int *btf_id = &fn->btf_id[arg];
> > > > > -     int ret;
> > > > > -
> > > > > -     if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
> > > > > +     if (!id || id > btf_vmlinux->nr_types)
> > > > >                return -EINVAL;
> > > >
> > > > id == 0 if btf_id cannot be resolved by resolve_btfids, right?
> > > > when id may be greater than btf_vmlinux->nr_types? If resolve_btfids
> > > > application did incorrect transformation?
> > > >
> > > > Anyway, this is to resolve helper meta btf_id. Even if you
> > > > return a btf_id > btf_vmlinux->nr_types, verifier will reject
> > > > since it will never be the same as the real parameter btf_id.
> > > > I would drop id > btf_vmlinux->nr_types here. This should never
> > > > happen for a correct tool. Even if it does, verifier will take
> > > > care of it.
> > > >
> > >
> > > I'd love to hear Alexei's thoughts about this change as well. Jiri
> > > removed not just BTF ID resolution, but also all the sanity checks.
> > > This now means more trust in helper definitions to not screw up
> > > anything. It's probably OK, but still something to consciously think
> > > about.
> >
> > The kernel will have to trust the result.
>
> +1
> I think 'if (!id || id > btf_vmlinux->nr_types)' at run-time and
> other sanity checks I saw in other patches are unnecessary.
> resolve_btfids should do all checks and fail vmlinux linking.
> We trust gcc to generate correct assembly code in the first place
> and correct dwarf. We trust pahole do correct BTF conversion from
> dwarf and dedup. We should trust resolve_btfids.
> It's imo the simplest tool comparing to gcc.
> btf_parse_vmlinux() is doing basic sanity check of BTF mainly
> to populate 'struct btf *btf_vmlinux;' for further use.
> I think we can add a scan over resolved btfids to btf_parse_vmlinux()
> to make sure that all ids are within the range, but I don't think
> it's mandatory for this patch set. Would be a reasonable sanity
> check for the future. Of course, checking for sorted set_start/end
> is overkill. Basic simplest sanity is ok.

My point was *not* about trusting resolve_btfids tool, it was about
trusting BPF helper definitions. But it's more of help for developer
while developing new helpers, so it's not a major thing.