netdev - Re: [PATCH v2 bpf-next 04/17] libbpf: mark BPF subprogs with hidden visibility as static for BPF verifier

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 16:28:42 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     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 04/17] libbpf: mark BPF subprogs with hidden
 visibility as static for BPF verifier

On Thu, Apr 22, 2021 at 4:00 PM Yonghong Song <yhs@...com> wrote:
>
>
>
> On 4/22/21 11:09 AM, Andrii Nakryiko wrote:
> > On Wed, Apr 21, 2021 at 10:43 PM Yonghong Song <yhs@...com> wrote:
> >>
> >>
> >>
> >> On 4/16/21 1:23 PM, Andrii Nakryiko wrote:
> >>> Define __hidden helper macro in bpf_helpers.h, which is a short-hand for
> >>> __attribute__((visibility("hidden"))). Add libbpf support to mark BPF
> >>> subprograms marked with __hidden as static in BTF information to enforce BPF
> >>> verifier's static function validation algorithm, which takes more information
> >>> (caller's context) into account during a subprogram validation.
> >>>
> >>> Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> >>> ---
> >>>    tools/lib/bpf/bpf_helpers.h     |  8 ++++++
> >>>    tools/lib/bpf/btf.c             |  5 ----
> >>>    tools/lib/bpf/libbpf.c          | 45 ++++++++++++++++++++++++++++++++-
> >>>    tools/lib/bpf/libbpf_internal.h |  6 +++++
> >>>    4 files changed, 58 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> >>> index 75c7581b304c..9720dc0b4605 100644
> >>> --- a/tools/lib/bpf/bpf_helpers.h
> >>> +++ b/tools/lib/bpf/bpf_helpers.h
> >>> @@ -47,6 +47,14 @@
> >>>    #define __weak __attribute__((weak))
> >>>    #endif
> >>>
> >>> +/*
> >>> + * Use __hidden attribute to mark a non-static BPF subprogram effectively
> >>> + * static for BPF verifier's verification algorithm purposes, allowing more
> >>> + * extensive and permissive BPF verification process, taking into account
> >>> + * subprogram's caller context.
> >>> + */
> >>> +#define __hidden __attribute__((visibility("hidden")))
> >>
> >> To prevent potential external __hidden macro definition conflict, how
> >> about
> >>
> >> #ifdef __hidden
> >> #undef __hidden
> >> #define __hidden __attribute__((visibility("hidden")))
> >> #endif
> >>
> >
> > We do force #undef only with __always_inline because of the bad
> > definition in linux/stddef.h And we check #ifndef for __weak, because
> > __weak is defined in kernel headers. This is not really the case for
> > __hidden, the only definition is in
> > tools/lib/traceevent/event-parse-local.h, which I don't think we
> > should worry about in BPF context. So I wanted to keep it simple and
> > fix only if that really causes some real conflicts.
> >
> > And keep in mind that in BPF code bpf_helpers.h is usually included as
> > one of the first few headers anyways.
>
> That is fine. Conflict of __hidden is a low risk and we can deal with it
> later if needed.
>
> >
> >
> >>> +
> >>>    /* When utilizing vmlinux.h with BPF CO-RE, user BPF programs can't include
> >>>     * any system-level headers (such as stddef.h, linux/version.h, etc), and
> >>>     * commonly-used macros like NULL and KERNEL_VERSION aren't available through
> >
> > [...]
> >
> >>> @@ -698,6 +700,15 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
> >>>                if (err)
> >>>                        return err;
> >>>
> >>> +             /* if function is a global/weak symbol, but has hidden
> >>> +              * visibility (or any non-default one), mark its BTF FUNC as
> >>> +              * static to enable more permissive BPF verification mode with
> >>> +              * more outside context available to BPF verifier
> >>> +              */
> >>> +             if (GELF_ST_BIND(sym.st_info) != STB_LOCAL
> >>> +                 && GELF_ST_VISIBILITY(sym.st_other) != STV_DEFAULT)
> >>
> >> Maybe we should check GELF_ST_VISIBILITY(sym.st_other) == STV_HIDDEN
> >> instead?
> >
> > It felt like only STV_DEFAULT should be "exported", semantically
> > speaking. Everything else would be treated as if it was static, except
> > that C rules require that function has to be global. Do you think
> > there is some danger to do it this way?
> >
> > Currently static linker doesn't do anything special for STV_INTERNAL
> > and STV_PROTECTED, so we could just disable those. Do you prefer that?
>
> Yes, let us just deal with STV_DEFAULT and STV_HIDDEN. We already
> specialized STV_HIDDEN, so we should not treat STV_INTERNAL/PROTECTED
> as what they mean in ELF standard, so let us disable them for now.

Yep, will do

>
> >
> > I just felt that there is no risk of regression if we do this for
> > non-STV_DEFAULT generically.
> >
> >
> >>
> >>> +                     prog->mark_btf_static = true;
> >>> +
> >>>                nr_progs++;
> >>>                obj->nr_programs = nr_progs;
> >>>
> >
> > [...]
> >