[bpf-next,V3] bpf: avoid UB in usages of the __imm_insn macro

Commit Message

Jose E. Marchesi May 8, 2024, 10:35 a.m. UTC

[Changes from V2:
 - no-strict-aliasing is only applied when building with GCC.
 - cpumask_failure.c is excluded, as it doesn't use __imm_insn.]

The __imm_insn macro is defined in bpf_misc.h as:

  #define __imm_insn(name, expr) [name]"i"(*(long *)&(expr))

This may lead to type-punning and strict aliasing rules violations in
it's typical usage where the address of a struct bpf_insn is passed as
expr, like in:

  __imm_insn(st_mem,
             BPF_ST_MEM(BPF_W, BPF_REG_1, offsetof(struct __sk_buff, mark), 42))

Where:

  #define BPF_ST_MEM(SIZE, DST, OFF, IMM)				\
	((struct bpf_insn) {					\
		.code  = BPF_ST | BPF_SIZE(SIZE) | BPF_MEM,	\
		.dst_reg = DST,					\
		.src_reg = 0,					\
		.off   = OFF,					\
		.imm   = IMM })

In all the actual instances of this in the BPF selftests the value is
fed to a volatile asm statement as soon as it gets read from memory,
and thus it is unlikely anti-aliasing rules breakage may lead to
misguided optimizations.

However, GCC detects the potential problem (indirectly) by issuing a
warning stating that a temporary <Uxxxxxx> is used uninitialized,
where the temporary corresponds to the memory read by *(long *).

This patch adds -fno-strict-aliasing to the compilation flags of the
particular selftests that do type punning via __imm_insn, only for
GCC.

Tested in master bpf-next.
No regressions.

Signed-off-by: Jose E. Marchesi <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: Yonghong Song <[e-mail]>
Cc: Eduard Zingerman <[e-mail]>
---
 tools/testing/selftests/bpf/Makefile | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Yonghong Song May 8, 2024, 3:38 p.m. UTC | #1

On 5/8/24 3:35 AM, Jose E. Marchesi wrote:
> [Changes from V2:
>   - no-strict-aliasing is only applied when building with GCC.
>   - cpumask_failure.c is excluded, as it doesn't use __imm_insn.]
>
> The __imm_insn macro is defined in bpf_misc.h as:
>
>    #define __imm_insn(name, expr) [name]"i"(*(long *)&(expr))
>
> This may lead to type-punning and strict aliasing rules violations in
> it's typical usage where the address of a struct bpf_insn is passed as
> expr, like in:
>
>    __imm_insn(st_mem,
>               BPF_ST_MEM(BPF_W, BPF_REG_1, offsetof(struct __sk_buff, mark), 42))
>
> Where:
>
>    #define BPF_ST_MEM(SIZE, DST, OFF, IMM)				\
> 	((struct bpf_insn) {					\
> 		.code  = BPF_ST | BPF_SIZE(SIZE) | BPF_MEM,	\
> 		.dst_reg = DST,					\
> 		.src_reg = 0,					\
> 		.off   = OFF,					\
> 		.imm   = IMM })
>
> In all the actual instances of this in the BPF selftests the value is
> fed to a volatile asm statement as soon as it gets read from memory,
> and thus it is unlikely anti-aliasing rules breakage may lead to
> misguided optimizations.
>
> However, GCC detects the potential problem (indirectly) by issuing a
> warning stating that a temporary <Uxxxxxx> is used uninitialized,
> where the temporary corresponds to the memory read by *(long *).
>
> This patch adds -fno-strict-aliasing to the compilation flags of the
> particular selftests that do type punning via __imm_insn, only for
> GCC.
>
> Tested in master bpf-next.
> No regressions.
>
> Signed-off-by: Jose E. Marchesi <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Yonghong Song <[e-mail]>
> Cc: Eduard Zingerman <[e-mail]>

Acked-by: Yonghong Song <[e-mail]>

[e-mail] May 8, 2024, 5 p.m. UTC | #2

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <[e-mail]>:

On Wed,  8 May 2024 12:35:51 +0200 you wrote:
> [Changes from V2:
>  - no-strict-aliasing is only applied when building with GCC.
>  - cpumask_failure.c is excluded, as it doesn't use __imm_insn.]
> 
> The __imm_insn macro is defined in bpf_misc.h as:
> 
>   #define __imm_insn(name, expr) [name]"i"(*(long *)&(expr))
> 
> [...]

Here is the summary with links:
  - [bpf-next,V3] bpf: avoid UB in usages of the __imm_insn macro
    https://git.kernel.org/bpf/bpf-next/c/1209a523f691

You are awesome, thank you!

Patch

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 511ea84139a3..e962a0bd8a78 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -86,6 +86,19 @@  progs/btf_dump_test_case_namespacing.c-bpf_gcc-CFLAGS := -Wno-error
 progs/btf_dump_test_case_packing.c-bpf_gcc-CFLAGS := -Wno-error
 progs/btf_dump_test_case_padding.c-bpf_gcc-CFLAGS := -Wno-error
 progs/btf_dump_test_case_syntax.c-bpf_gcc-CFLAGS := -Wno-error
+
+# The following tests do type-punning, via the __imm_insn macro, from
+# `struct bpf_insn' to long and then uses the value.  This triggers an
+# "is used uninitialized" warning in GCC due to strict-aliasing
+# rules.
+progs/verifier_ref_tracking.c-bpf_gcc-CFLAGS := -fno-strict-aliasing
+progs/verifier_unpriv.c-bpf_gcc-CFLAGS := -fno-strict-aliasing
+progs/verifier_cgroup_storage.c-bpf_gcc-CFLAGS := -fno-strict-aliasing
+progs/verifier_ld_ind.c-bpf_gcc-CFLAGS := -fno-strict-aliasing
+progs/verifier_map_ret_val.c-bpf_gcc-CFLAGS := -fno-strict-aliasing
+progs/verifier_spill_fill.c-bpf_gcc-CFLAGS := -fno-strict-aliasing
+progs/verifier_subprog_precision.c-bpf_gcc-CFLAGS := -fno-strict-aliasing
+progs/verifier_uninit.c-bpf_gcc-CFLAGS := -fno-strict-aliasing
 endif
 
 ifneq ($(CLANG_CPUV4),)