[PATCH bpf-next V2] bpf: avoid UB in usages of the __imm_insn macro

[PATCH bpf-next V2] bpf: avoid UB in usages of the __imm_insn macro

[Jose E. Marchesi]

[Differences with V1:
- Typo fixed in patch: progs/verifier_ref_tracking.c
  was missing -CFLAGS.]

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 })

GCC detects this 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.  This
silences the warning and, most importantly, avoids potential
optimization problems due to breaking anti-aliasing rules.

Tested in master bpf-next.
No regressions.

Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
Cc: david.faust@oracle.com
Cc: cupertino.miranda@oracle.com
Cc: Yonghong Song <yonghong.song@linux.dev>
Cc: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/Makefile | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index f0c429cf4424..c7507f420d9e 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -53,6 +53,21 @@ progs/syscall.c-CFLAGS := -fno-strict-aliasing
 progs/test_pkt_md_access.c-CFLAGS := -fno-strict-aliasing
 progs/test_sk_lookup.c-CFLAGS := -fno-strict-aliasing
 progs/timer_crash.c-CFLAGS := -fno-strict-aliasing
+# In the following tests the strict aliasing rules are broken by the
+# __imm_insn macro, that do type-punning from `struct bpf_insn' to
+# long and then uses the value.  This triggers an "is used
+# uninitialized" warning in GCC.  This in theory may also lead to
+# broken programs, so it is better to disable strict aliasing than
+# inhibiting the warning.
+progs/verifier_ref_tracking.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_unpriv.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_cgroup_storage.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_ld_ind.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_map_ret_val.c-CFLAGS := -fno-strict-aliasing
+progs/cpumask_failure.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_spill_fill.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_subprog_precision.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_uninit.c-CFLAGS := -fno-strict-aliasing
 
 ifneq ($(LLVM),)
 # Silence some warnings when compiled with clang
-- 
2.30.2

Re: [PATCH bpf-next V2] bpf: avoid UB in usages of the __imm_insn macro

[Yonghong Song]

On 5/7/24 6:31 AM, Jose E. Marchesi wrote:
> [Differences with V1:
> - Typo fixed in patch: progs/verifier_ref_tracking.c
>    was missing -CFLAGS.]
>
> 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 })
>
> GCC detects this 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.  This
> silences the warning and, most importantly, avoids potential
> optimization problems due to breaking anti-aliasing rules.

For all the modified verifier_* files below, the functions
are naked inline asm, so there is no optimization risk of breaking
anti-aliasing rules. Is this right?

>
> Tested in master bpf-next.
> No regressions.
>
> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
> Cc: david.faust@oracle.com
> Cc: cupertino.miranda@oracle.com
> Cc: Yonghong Song <yonghong.song@linux.dev>
> Cc: Eduard Zingerman <eddyz87@gmail.com>
> ---
>   tools/testing/selftests/bpf/Makefile | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index f0c429cf4424..c7507f420d9e 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -53,6 +53,21 @@ progs/syscall.c-CFLAGS := -fno-strict-aliasing
>   progs/test_pkt_md_access.c-CFLAGS := -fno-strict-aliasing
>   progs/test_sk_lookup.c-CFLAGS := -fno-strict-aliasing
>   progs/timer_crash.c-CFLAGS := -fno-strict-aliasing
> +# In the following tests the strict aliasing rules are broken by the
> +# __imm_insn macro, that do type-punning from `struct bpf_insn' to
> +# long and then uses the value.  This triggers an "is used
> +# uninitialized" warning in GCC.  This in theory may also lead to
> +# broken programs, so it is better to disable strict aliasing than
> +# inhibiting the warning.
> +progs/verifier_ref_tracking.c-CFLAGS := -fno-strict-aliasing
> +progs/verifier_unpriv.c-CFLAGS := -fno-strict-aliasing
> +progs/verifier_cgroup_storage.c-CFLAGS := -fno-strict-aliasing
> +progs/verifier_ld_ind.c-CFLAGS := -fno-strict-aliasing
> +progs/verifier_map_ret_val.c-CFLAGS := -fno-strict-aliasing
> +progs/cpumask_failure.c-CFLAGS := -fno-strict-aliasing

All these verifier_* files have __imm_insn, but I didn't see
__imm_insn usage for cpumask_failure.c. Did I miss anything?

All these verifier_* files are naked inline asm. So it should not
cause any issues with -fstrict-aliasing. Since there are no
issues for clang. Maybe just add -fno-strict-aliasing for gcc
only to silence the warning.

> +progs/verifier_spill_fill.c-CFLAGS := -fno-strict-aliasing
> +progs/verifier_subprog_precision.c-CFLAGS := -fno-strict-aliasing
> +progs/verifier_uninit.c-CFLAGS := -fno-strict-aliasing
>   
>   ifneq ($(LLVM),)
>   # Silence some warnings when compiled with clang

Re: [PATCH bpf-next V2] bpf: avoid UB in usages of the __imm_insn macro

[Jose E. Marchesi]

> On 5/7/24 6:31 AM, Jose E. Marchesi wrote:
>> [Differences with V1:
>> - Typo fixed in patch: progs/verifier_ref_tracking.c
>>    was missing -CFLAGS.]
>>
>> 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 })
>>
>> GCC detects this 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.  This
>> silences the warning and, most importantly, avoids potential
>> optimization problems due to breaking anti-aliasing rules.
>
> For all the modified verifier_* files below, the functions
> are naked inline asm, so there is no optimization risk of breaking
> anti-aliasing rules. Is this right?

I think you are right, in these particular functions, since the result
of the memory read cannot be discarded as the asm uses it.

>
>>
>> Tested in master bpf-next.
>> No regressions.
>>
>> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
>> Cc: david.faust@oracle.com
>> Cc: cupertino.miranda@oracle.com
>> Cc: Yonghong Song <yonghong.song@linux.dev>
>> Cc: Eduard Zingerman <eddyz87@gmail.com>
>> ---
>>   tools/testing/selftests/bpf/Makefile | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>> index f0c429cf4424..c7507f420d9e 100644
>> --- a/tools/testing/selftests/bpf/Makefile
>> +++ b/tools/testing/selftests/bpf/Makefile
>> @@ -53,6 +53,21 @@ progs/syscall.c-CFLAGS := -fno-strict-aliasing
>>   progs/test_pkt_md_access.c-CFLAGS := -fno-strict-aliasing
>>   progs/test_sk_lookup.c-CFLAGS := -fno-strict-aliasing
>>   progs/timer_crash.c-CFLAGS := -fno-strict-aliasing
>> +# In the following tests the strict aliasing rules are broken by the
>> +# __imm_insn macro, that do type-punning from `struct bpf_insn' to
>> +# long and then uses the value.  This triggers an "is used
>> +# uninitialized" warning in GCC.  This in theory may also lead to
>> +# broken programs, so it is better to disable strict aliasing than
>> +# inhibiting the warning.
>> +progs/verifier_ref_tracking.c-CFLAGS := -fno-strict-aliasing
>> +progs/verifier_unpriv.c-CFLAGS := -fno-strict-aliasing
>> +progs/verifier_cgroup_storage.c-CFLAGS := -fno-strict-aliasing
>> +progs/verifier_ld_ind.c-CFLAGS := -fno-strict-aliasing
>> +progs/verifier_map_ret_val.c-CFLAGS := -fno-strict-aliasing
>> +progs/cpumask_failure.c-CFLAGS := -fno-strict-aliasing
>
> All these verifier_* files have __imm_insn, but I didn't see
> __imm_insn usage for cpumask_failure.c. Did I miss anything?
>
> All these verifier_* files are naked inline asm. So it should not
> cause any issues with -fstrict-aliasing. Since there are no
> issues for clang. Maybe just add -fno-strict-aliasing for gcc
> only to silence the warning.

Ok.

I will send a V2 as soon as Cupertino's patch adding support for
-bpf_gcc-CFLAGS gets applied upstream.

Thanks.

>
>> +progs/verifier_spill_fill.c-CFLAGS := -fno-strict-aliasing
>> +progs/verifier_subprog_precision.c-CFLAGS := -fno-strict-aliasing
>> +progs/verifier_uninit.c-CFLAGS := -fno-strict-aliasing
>>     ifneq ($(LLVM),)
>>   # Silence some warnings when compiled with clang

Re: [PATCH bpf-next V2] bpf: avoid UB in usages of the __imm_insn macro

[Jose E. Marchesi]

> On 5/7/24 6:31 AM, Jose E. Marchesi wrote:
>> [Differences with V1:
>> - Typo fixed in patch: progs/verifier_ref_tracking.c
>>    was missing -CFLAGS.]
>>
>> 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 })
>>
>> GCC detects this 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.  This
>> silences the warning and, most importantly, avoids potential
>> optimization problems due to breaking anti-aliasing rules.
>
> For all the modified verifier_* files below, the functions
> are naked inline asm, so there is no optimization risk of breaking
> anti-aliasing rules. Is this right?
>
>>
>> Tested in master bpf-next.
>> No regressions.
>>
>> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
>> Cc: david.faust@oracle.com
>> Cc: cupertino.miranda@oracle.com
>> Cc: Yonghong Song <yonghong.song@linux.dev>
>> Cc: Eduard Zingerman <eddyz87@gmail.com>
>> ---
>>   tools/testing/selftests/bpf/Makefile | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>> index f0c429cf4424..c7507f420d9e 100644
>> --- a/tools/testing/selftests/bpf/Makefile
>> +++ b/tools/testing/selftests/bpf/Makefile
>> @@ -53,6 +53,21 @@ progs/syscall.c-CFLAGS := -fno-strict-aliasing
>>   progs/test_pkt_md_access.c-CFLAGS := -fno-strict-aliasing
>>   progs/test_sk_lookup.c-CFLAGS := -fno-strict-aliasing
>>   progs/timer_crash.c-CFLAGS := -fno-strict-aliasing
>> +# In the following tests the strict aliasing rules are broken by the
>> +# __imm_insn macro, that do type-punning from `struct bpf_insn' to
>> +# long and then uses the value.  This triggers an "is used
>> +# uninitialized" warning in GCC.  This in theory may also lead to
>> +# broken programs, so it is better to disable strict aliasing than
>> +# inhibiting the warning.
>> +progs/verifier_ref_tracking.c-CFLAGS := -fno-strict-aliasing
>> +progs/verifier_unpriv.c-CFLAGS := -fno-strict-aliasing
>> +progs/verifier_cgroup_storage.c-CFLAGS := -fno-strict-aliasing
>> +progs/verifier_ld_ind.c-CFLAGS := -fno-strict-aliasing
>> +progs/verifier_map_ret_val.c-CFLAGS := -fno-strict-aliasing
>> +progs/cpumask_failure.c-CFLAGS := -fno-strict-aliasing
>
> All these verifier_* files have __imm_insn, but I didn't see
> __imm_insn usage for cpumask_failure.c. Did I miss anything?

Sorry, I missed this question.  cpumask_failure.c wasn't meant to be
there.  Will omit it in the V2 of the patch.

>
> All these verifier_* files are naked inline asm. So it should not
> cause any issues with -fstrict-aliasing. Since there are no
> issues for clang. Maybe just add -fno-strict-aliasing for gcc
> only to silence the warning.
>
>> +progs/verifier_spill_fill.c-CFLAGS := -fno-strict-aliasing
>> +progs/verifier_subprog_precision.c-CFLAGS := -fno-strict-aliasing
>> +progs/verifier_uninit.c-CFLAGS := -fno-strict-aliasing
>>     ifneq ($(LLVM),)
>>   # Silence some warnings when compiled with clang