linux-kernel - Re: [PATCH bpf-next v9 2/2] arm64/cfi,bpf: Support kCFI + BPF on arm64

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] [day] [month] [year] [list]

Message-ID: <aHOSCtykfYLLmy1n@willie-the-truck>
Date: Sun, 13 Jul 2025 12:01:30 +0100
From: Will Deacon <will@...nel.org>
To: Sami Tolvanen <samitolvanen@...gle.com>
Cc: bpf@...r.kernel.org, Puranjay Mohan <puranjay@...nel.org>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Catalin Marinas <catalin.marinas@....com>,
	Andrii Nakryiko <andrii@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Maxwell Bland <mbland@...orola.com>,
	Puranjay Mohan <puranjay12@...il.com>,
	Dao Huang <huangdao1@...o.com>
Subject: Re: [PATCH bpf-next v9 2/2] arm64/cfi,bpf: Support kCFI + BPF on
 arm64

Hey Sami,

On Fri, Jul 11, 2025 at 11:49:29AM -0700, Sami Tolvanen wrote:
> > > +#define cfi_get_offset cfi_get_offset
> > > +extern u32 cfi_bpf_hash;
> > > +extern u32 cfi_bpf_subprog_hash;
> > > +extern u32 cfi_get_func_hash(void *func);
> > > +#else
> > > +#define cfi_bpf_hash 0U
> > > +#define cfi_bpf_subprog_hash 0U
> > > +static inline u32 cfi_get_func_hash(void *func)
> > > +{
> > > +     return 0;
> > > +}
> > > +#endif /* CONFIG_CFI_CLANG */
> > > +#endif /* _ASM_ARM64_CFI_H */
> >
> > This looks like an awful lot of boiler plate to me. The only thing you
> > seem to need is the CFI offset -- why isn't that just a constant that we
> > can define (or a Kconfig symbol?).
> 
> The cfi_get_offset function was originally added in commit
> 4f9087f16651 ("x86/cfi,bpf: Fix BPF JIT call") because the offset can
> change on x86 depending on which CFI scheme is enabled at runtime.
> Starting with commit 2cd3e3772e41 ("x86/cfi,bpf: Fix bpf_struct_ops
> CFI") the function is also called by the generic BPF code, so we can't
> trivially replace it with a constant. However, since this defaults to
> `4` unless the architecture adds pre-function NOPs, I think we could
> simply move the default implementation to include/linux/cfi.h (and
> also drop the RISC-V version). Come to think of it, we could probably
> move most of this boilerplate to generic code. I'll refactor this and
> send a new version.

Excellent, thanks.

> > > @@ -2009,9 +2018,9 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> > >               jit_data->ro_header = ro_header;
> > >       }
> > >
> > > -     prog->bpf_func = (void *)ctx.ro_image;
> > > +     prog->bpf_func = (void *)ctx.ro_image + cfi_get_offset();
> > >       prog->jited = 1;
> > > -     prog->jited_len = prog_size;
> > > +     prog->jited_len = prog_size - cfi_get_offset();
> >
> > Why do we add the offset even when CONFIG_CFI_CLANG is not enabled?
> 
> The function returns zero if CFI is not enabled, so I believe it's
> just to avoid extra if (IS_ENABLED(CONFIG_CFI_CLANG)) statements in
> the code. IMO this is cleaner, but I can certainly change this if you
> prefer.

Ah, that caught me out because the !CONFIG_CFI_CLANG stub is in the
core code (and I'm extra susceptible to being caught out on a warm
Friday evening!).

Hopefully if you're able to trim down the boilerplate then this will
become more obvious too.

Cheers,

Will