| 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
| ||
|
Message-ID: <f442380c2d8cc51b38105c6316cbe224a248fdfe.camel@intel.com>
Date: Wed, 4 Jun 2025 23:16:16 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "tglx@...utronix.de" <tglx@...utronix.de>, "peterz@...radead.org"
<peterz@...radead.org>, "mingo@...hat.com" <mingo@...hat.com>, "Hansen, Dave"
<dave.hansen@...el.com>, "kirill.shutemov@...ux.intel.com"
<kirill.shutemov@...ux.intel.com>, "bp@...en8.de" <bp@...en8.de>,
"hpa@...or.com" <hpa@...or.com>
CC: "samitolvanen@...gle.com" <samitolvanen@...gle.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Edgecombe,
Rick P" <rick.p.edgecombe@...el.com>, "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH] x86/virt/tdx: Add ENDBR for low level SEAMCALL assembly
functions
On Wed, 2025-06-04 at 12:46 -0700, Dave Hansen wrote:
> On 6/3/25 17:38, Kai Huang wrote:
> > Build warnings about missing ENDBR around making SEAMCALLs[*] were
> > observed when using some randconfig[1] to build today's Linus's tree.
>
> What actually caused it? CONFIG_CC_OPTIMIZE_FOR_SIZE?
Yes. I just verified that building with "-O2" doesn't have this warning but
"-Os" does.
I spent hours but didn't figure this out. So, thank you!
>
> > In the C code, the low level SEAMCALL assembly functions (__seamcall(),
> > __seamcall_ret() and __seamcall_saved_ret()) are indirectly called via
> > the common sc_retry() function:
> >
> > static inline u64 sc_retry(sc_func_t func, u64 fn,
> > struct tdx_module_args *args)
> > { ... }
> >
> > #define seamcall(_fn, _args) sc_retry(__seamcall, (_fn), (_args))
> >
> > It turns out compilers may not always be smart enough to figure out how
> > to call those assembly functions directly.
>
> So, reading this, it didn't quite click into my brain that you were
> referring to "direct call" and "indirect call" *instructions*.
>
> I think I probably would have talked about "function pointers" because
> that's what you do in C. A function pointer mostly means an indirect
> call. But not always, like was intended here.
I was thinking the function pointers are passed as function argument when
sc_retry() gets called, so maybe in some cases that I don't know the
compiler just cannot figure out exactly what those function pointers are.
>
> > The disassembly of the vmlinux built from the aforementioned config
> > confirms that __seamcall*() are indirectly called:
> >
> > <sc_retry>:
> > ......
> >
> > 4c 89 ee mov %r13,%rsi
> > 4c 89 e7 mov %r12,%rdi
> > e8 35 8c 7d 01 call ffffffff82b3e220 <__pi___x86_indirect_thunk_rbp>
> > 4c 39 f0 cmp %r14,%rax
> >
> > In this case ENDBR is needed at the beginning of __seamcall*().
> >
> > Change SYM_FUNC_START() to SYM_TYPED_FUNC_START() for __seamcall*() to
> > add ENDBR to them.
>
> ... and why is this? How do we know this is the correct fix? Show your
> work, please.
I don't know details of how CFI works, but my understanding is
SYM_TYPED_FUNC_START() is the standard one for indirectly called globals
with CFI handled:
<asm/linkage.h>:
/* SYM_TYPED_FUNC_START -- use for indirectly called globals, w/ CFI type */
#define SYM_TYPED_FUNC_START(name) \
SYM_TYPED_START(name, SYM_L_GLOBAL, SYM_F_ALIGN) \
ENDBR
And the SYM_TYPED_START() handles CFI for CLANG:
...
#define __CFI_TYPE(name) \
SYM_START(__cfi_##name, SYM_L_LOCAL, SYM_A_NONE) \
CFI_PRE_PADDING \
.byte 0xb8 ASM_NL \
.long __kcfi_typeid_##name ASM_NL \
CFI_POST_PADDING \
SYM_FUNC_END(__cfi_##name)
...
<linux/cfi_types.h>:
...
#define SYM_TYPED_ENTRY(name, linkage, align...) \
linkage(name) ASM_NL \
align ASM_NL \
__CFI_TYPE(name) ASM_NL \
name:
#define SYM_TYPED_START(name, linkage, align...) \
SYM_TYPED_ENTRY(name, linkage, align)
...
The ENDBR right after "SYM_TYPED_START()" is for IBT (which IIUC is
independent to CFI CLANG as you mentioned below).
But I confess I don't know details about CFI so perhaps Peter could help to
confirm?
>
> > When the compiler can generate direct call for __seamcall*(), the
> > additional ENDBR is safe since it has no impact to directly called
> > functions.
>
> Right. Direct calls are always OK. Indirect call targets need to be
> handled specially.
>
> > When kernel IBT was added to the kernel, initially the SYM_FUNC_START()
> > had the ENDBR added in commit
> >
> > c4691712b546 ("x86/linkage: Add ENDBR to SYM_FUNC_START*()")
> >
> > However when the commit
> >
> > 582077c94052 ("x86/cfi: Clean up linkage")
> >
> > removed the ENDBR from the SYM_FUNC_START() and added it to the
> > SYM_TYPED_FUNC_START(), it didn't touch the SEAMCALL assembly.
>
> I'm not sure why we need the history lesson here.
I dig the history mainly because I thought we might need a "Fixes" tag.
And I then conclude the commit 582077c94052 ("x86/cfi: Clean up linkage")
seems to be the one broke it. So to justify the Fixes tag, I added the
history here.
>
> > [*] Aforementioned build warning:
> >
> > vmlinux.o: warning: objtool: try_init_module_global+0x5d: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: read_sys_metadata_field+0x4a: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: do_global_key_config+0x36: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_phymem_page_reclaim+0x71: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_phymem_cache_wb+0x41: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_phymem_page_wbinvd_tdr+0x95: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdx_cpu_enable+0x7b: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: init_tdmr+0x59: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: config_tdx_module.constprop.0+0x19d: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_vp_addcx+0x91: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_vp_init+0x76: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_vp_wr+0x87: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_vp_rd+0x6d: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_vp_flush+0x4c: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_vp_create+0x85: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mng_create+0x73: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mem_page_aug+0xb4: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mem_sept_add+0xb4: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mem_page_add+0xce: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mng_addcx+0x91: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mem_page_remove+0x7e: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mem_track+0x4c: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mng_init+0x6d: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mng_key_freeid+0x4c: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mng_vpflushdone+0x4c: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mr_finalize+0x4c: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mr_extend+0x77: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mng_rd+0x6d: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mng_key_config+0x4c: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mem_range_block+0x7e: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_phymem_page_wbinvd_hkid+0x7d: relocation to !ENDBR: __seamcall+0x0
>
> One or two of those would have sufficed...
Will keep this in mind. Thanks.
>
> > diff --git a/arch/x86/virt/vmx/tdx/seamcall.S b/arch/x86/virt/vmx/tdx/seamcall.S
> > index 6854c52c374b..637226ae935d 100644
> > --- a/arch/x86/virt/vmx/tdx/seamcall.S
> > +++ b/arch/x86/virt/vmx/tdx/seamcall.S
> > @@ -1,5 +1,6 @@
> > /* SPDX-License-Identifier: GPL-2.0 */
> > #include <linux/linkage.h>
> > +#include <linux/cfi_types.h>
> > #include <asm/frame.h>
> >
> > #include "tdxcall.S"
> > @@ -18,7 +19,7 @@
> > * Return (via RAX) TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself
> > * fails, or the completion status of the SEAMCALL leaf function.
> > */
> > -SYM_FUNC_START(__seamcall)
> > +SYM_TYPED_FUNC_START(__seamcall)
> > TDX_MODULE_CALL host=1
> > SYM_FUNC_END(__seamcall)
> Personally, I'd add a _bit_ to these comments to mention that these
> functions can be called via an indirect call.
There are three comments since we have three helpers.
I am not sure duplicating the "indirect call" in those three comments is
good idea? Or perhaps we can just add an overall comment at the beginning
of those three helpers like below?
diff --git a/arch/x86/virt/vmx/tdx/seamcall.S
b/arch/x86/virt/vmx/tdx/seamcall.S
index 6854c52c374b..a78e46ec4a12 100644
--- a/arch/x86/virt/vmx/tdx/seamcall.S
+++ b/arch/x86/virt/vmx/tdx/seamcall.S
@@ -4,6 +4,11 @@
#include "tdxcall.S"
+/*
+ * Use SYM_TYPED_FUNC_START() for those functions since they can be
+ * called via an indirect call.
+ */
+
/*
* __seamcall() - Host-side interface functions to SEAM software
* (the P-SEAMLDR or the TDX module).
>
> Circling back around now that I've divined out what is happening here...
> Is this even really about ENDBR at all? I thought that clang has some
> CFI checking that's completely independent from CET/IBT and thus ENDBR.
> Wouldn't this also break with that?
It doesn't seem this would break anything to me, as replied above.
But it would be great if Peter can jump in to confirm.
>
> At the risk of giving a man a fish...
>
Thanks for the fish. :-)
> Isn't this a much more coherent
> changelog at about 1/4 the size?
Yes it's more concise. I'll use it in next version.
Could you also let me know whether I should keep the Fixes tag, and the
history in the changelog?
Thanks.
>
> --
>
> Subject: x86/virt/tdx: Annotate TDX assembly to allow indirect calls
>
> A TDX helper function (sc_retry()) passes around function pointers to
> assembly functions. Normally, the compiler realizes that the function
> pointer targets are completely static, can be resolved at compile time,
> and generates direct call instructions.
>
> But, other times (like when CONFIG_CC_OPTIMIZE_FOR_SIZE=y), the compiler
> will instead generate indirect call instructions.
>
> Indirect calls to assembly functions require special annotation so that
> both hardware and software implementations of Control Flow Integrity
> mechanisms can work correctly.
>
> The TDX functions are declared as if they are only called directly (via
> SYM_FUNC_START). Move them over to another macro (SYM_TYPED_FUNC_START)
> which will annotate them as being called indirectly (see
> include/linux/cfi_types.h).
>
> This was found through randconfig testing, presumably setting
> CONFIG_CC_OPTIMIZE_FOR_SIZE=1 when objtool spewed a bunch of these:
>
> vmlinux.o: warning: objtool: tdh_mem_range_block+0x7e: relocation to
> !ENDBR: __seamcall_ret+0x0
>
Powered by blists - more mailing lists