linux-kernel - Re: [PATCH] bpf: don't rely on GCC __attribute_

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]

Message-ID: <72f0dd64-9f65-cbd0-873a-684540912847@iogearbox.net>
Date:   Wed, 28 Oct 2020 00:04:25 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Ard Biesheuvel <ardb@...nel.org>, linux-kernel@...r.kernel.org
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, arnd@...db.de,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Arvind Sankar <nivedita@...m.mit.edu>,
        Randy Dunlap <rdunlap@...radead.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Alexei Starovoitov <ast@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to
 disable GCSE

On 10/27/20 9:57 PM, Ard Biesheuvel wrote:
> Commit 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for
> ___bpf_prog_run()") introduced a __no_fgcse macro that expands to a
> function scope __attribute__((optimize("-fno-gcse"))), to disable a
> GCC specific optimization that was causing trouble on x86 builds, and
> was not expected to have any positive effect in the first place.
> 
> However, as the GCC manual documents, __attribute__((optimize))
> is not for production use, and results in all other optimization
> options to be forgotten for the function in question. This can
> cause all kinds of trouble, but in one particular reported case,

Looks like there are couple more as well aside from __no_fgcse, are you
also planning to fix them?

   arch/powerpc/kernel/setup.h:14:#define __nostackprotector __attribute__((__optimize__("no-stack-protector")))
   tools/include/linux/compiler-gcc.h:37:#define __no_tail_call	__attribute__((optimize("no-optimize-sibling-calls")))

> it causes -fno-asynchronous-unwind-tables to be disregarded,
> resulting in .eh_frame info to be emitted for the function
> inadvertently.

Would have been useful to add a pointer to the original discussion with
Link tag.

Link: https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/

> This reverts commit 3193c0836f203, and instead, it disables the -fgcse
> optimization for the entire source file, but only when building for
> X86.
> 
> Cc: Nick Desaulniers <ndesaulniers@...gle.com>
> Cc: Arvind Sankar <nivedita@...m.mit.edu>
> Cc: Randy Dunlap <rdunlap@...radead.org>
> Cc: Josh Poimboeuf <jpoimboe@...hat.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Alexei Starovoitov <ast@...nel.org>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> Cc: Peter Zijlstra (Intel) <peterz@...radead.org>
> Cc: Geert Uytterhoeven <geert@...ux-m68k.org>
> Cc: Kees Cook <keescook@...omium.org>
> Fixes: 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> Signed-off-by: Ard Biesheuvel <ardb@...nel.org>
[...]
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index bdc8cd1b6767..02b58f44c479 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -1,6 +1,8 @@
>   # SPDX-License-Identifier: GPL-2.0
>   obj-y := core.o
> -CFLAGS_core.o += $(call cc-disable-warning, override-init)
> +# ___bpf_prog_run() needs GCSE disabled on x86; see 3193c0836f203 for details
> +cflags-core-$(CONFIG_X86) := -fno-gcse
> +CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-core-y)

Also, this needs to be guarded behind !CONFIG_RETPOLINE and !CONFIG_BPF_JIT_ALWAYS_ON
in particular the latter since only in this case interpreter is compiled in ... most
distros have the CONFIG_BPF_JIT_ALWAYS_ON set these days for x86.

Do you have an analysis for the commit log on what else this penalizes in core.c if
it's now for the entire translation unit?

>   obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o tnum.o bpf_iter.o map_iter.o task_iter.o prog_iter.o
>   obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o map_in_map.o
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 9268d77898b7..55454d2278b1 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1369,7 +1369,7 @@ u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
>    *
>    * Decode and execute eBPF instructions.
>    */
> -static u64 __no_fgcse ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> +static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
>   {
>   #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
>   #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
>