netdev - Re: [PATCH v2 bpf-next 10/17] libbpf: tighten BTF type ID rewriting with error checking

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:   Thu, 22 Apr 2021 19:54:46 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Yonghong Song <yhs@...com>, Andrii Nakryiko <andrii@...nel.org>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 10/17] libbpf: tighten BTF type ID rewriting
 with error checking

On Thu, Apr 22, 2021 at 11:24 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Thu, Apr 22, 2021 at 9:50 AM Yonghong Song <yhs@...com> wrote:
> >
> >
> >
> > On 4/16/21 1:23 PM, Andrii Nakryiko wrote:
> > > It should never fail, but if it does, it's better to know about this rather
> > > than end up with nonsensical type IDs.
> >
> > So this is defensive programming. Maybe do another round of
> > audit of the callers and if you didn't find any issue, you
> > do not need to check not-happening condition here?
>
> It's far from obvious that this will never happen, because we do a
> decently complicated BTF processing (we skip some types altogether
> believing that they are not used, for example) and it will only get
> more complicated with time. Just as there are "verifier bug" checks in
> kernel, this prevents things from going wild if non-trivial bugs will
> inevitably happen.

I agree with Yonghong. This doesn't look right.
The callback will be called for all non-void types, right?
so *type_id == 0 shouldn't never happen.
If it does there is a bug somewhere that should be investigated
instead of ignored.
The
if (new_id == 0) pr_warn
bit makes sense.
My reading that it will abort the whole linking process and
this linker bug will be reported back to us.
So it's good.