From: "Jose E. Marchesi" <jose.marchesi@oracle.com>
To: Yonghong Song <yonghong.song@linux.dev>
Cc: bpf@vger.kernel.org, david.faust@oracle.com,
cupertino.miranda@oracle.com,
Eduard Zingerman <eddyz87@gmail.com>
Subject: Re: [PATCH bpf-next V2] bpf: avoid UB in usages of the __imm_insn macro
Date: Tue, 07 May 2024 21:18:33 +0200 [thread overview]
Message-ID: <87zft1bfcm.fsf@oracle.com> (raw)
In-Reply-To: <e76d3a47-fecf-4d2c-a417-9d1f5935df7a@linux.dev> (Yonghong Song's message of "Tue, 7 May 2024 11:54:20 -0700")
> 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
prev parent reply other threads:[~2024-05-07 19:18 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-07 13:31 [PATCH bpf-next V2] bpf: avoid UB in usages of the __imm_insn macro Jose E. Marchesi
2024-05-07 18:54 ` Yonghong Song
2024-05-07 19:17 ` Jose E. Marchesi
2024-05-07 19:18 ` Jose E. Marchesi [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zft1bfcm.fsf@oracle.com \
--to=jose.marchesi@oracle.com \
--cc=bpf@vger.kernel.org \
--cc=cupertino.miranda@oracle.com \
--cc=david.faust@oracle.com \
--cc=eddyz87@gmail.com \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).