| 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: <7523faad-23f0-2fcb-30e3-b0207d71e63f@suse.com>
Date: Thu, 7 Sep 2023 15:45:34 +0300
From: Nikolay Borisov <nik.borisov@...e.com>
To: Kai Huang <kai.huang@...el.com>, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org
Cc: x86@...nel.org, dave.hansen@...el.com,
kirill.shutemov@...ux.intel.com, tony.luck@...el.com,
peterz@...radead.org, tglx@...utronix.de, bp@...en8.de,
mingo@...hat.com, hpa@...or.com, seanjc@...gle.com,
pbonzini@...hat.com, david@...hat.com, dan.j.williams@...el.com,
rafael.j.wysocki@...el.com, ashok.raj@...el.com,
reinette.chatre@...el.com, len.brown@...el.com, ak@...ux.intel.com,
isaku.yamahata@...el.com, ying.huang@...el.com, chao.gao@...el.com,
sathyanarayanan.kuppuswamy@...ux.intel.com, bagasdotme@...il.com,
sagis@...gle.com, imammedo@...hat.com
Subject: Re: [PATCH v13 06/22] x86/virt/tdx: Add SEAMCALL error printing for
module initialization
On 25.08.23 г. 15:14 ч., Kai Huang wrote:
> TDX module initialization is essentially to make a set of SEAMCALL leafs
> to complete a state machine involving multiple states. These SEAMCALLs
> are not expected to fail. In fact, they are not expected to return any
> non-zero code (except the "running out of entropy error", which can be
> handled internally already).
>
> Add yet another layer of SEAMCALL wrappers, which treats all non-zero
> return code as error, to support printing SEAMCALL error upon failure
> for module initialization.
>
> Other SEAMCALLs may treat some specific error codes as legal (e.g., some
> can return BUSY legally and expect the caller to retry). The caller can
> use the wrappers w/o error printing for those cases. The new wrappers
> can also be improved to suit those cases. Leave this as future work.
>
> SEAMCALL can also return kernel defined error codes for three special
> cases: 1) TDX isn't enabled by the BIOS; 2) TDX module isn't loaded; 3)
> CPU isn't in VMX operation. The first case isn't expected (unless BIOS
> bug, etc) because SEAMCALL is only expected to be made when the kernel
> detects TDX is enabled. The second case is only expected to be legal
> for the very first SEAMCALL during module initialization. The third
> case can be legal for any SEAMCALL leaf because VMX can be disabled due
> to emergency reboot.
>
> Also add wrappers to convert the SEAMCALL error code to the kernel error
> code so that each caller doesn't have to repeat. Blindly print error
> for the above special cases to save the effort to optimize them.
>
> TDX module can only be initialized once during its life cycle, but the
> module can be runtime updated by the kernel (not yet supported). After
> module runtime update, the kernel needs to initialize it again. Use
> pr_err() to print SEAMCALL error for module initialization, because if
> using pr_err_once() the SEAMCALL error during module initialization
> won't be printed after module runtime update.
>
> At last, for now implement those wrappers in tdx.c but they can be moved
> to <asm/tdx.h> when needed. They are implemented with intention to be
> shared by other kernel components. After all, in most cases, SEAMCALL
> failure is unexpected and the caller just wants to print.
>
> Signed-off-by: Kai Huang <kai.huang@...el.com>
> ---
>
> v12 -> v13:
> - New implementation due to TDCALL assembly series.
>
> ---
> arch/x86/include/asm/tdx.h | 1 +
> arch/x86/virt/vmx/tdx/tdx.c | 84 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 85 insertions(+)
>
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index cfae8b31a2e9..3b248c94a4a4 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -27,6 +27,7 @@
> /*
> * TDX module SEAMCALL leaf function error codes
> */
> +#define TDX_SUCCESS 0ULL
> #define TDX_RND_NO_ENTROPY 0x8000020300000000ULL
>
> #ifndef __ASSEMBLY__
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 908590e85749..bb63cb7361c8 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -16,6 +16,90 @@
> #include <asm/msr.h>
> #include <asm/tdx.h>
>
> +#define seamcall_err(__fn, __err, __args, __prerr_func) \
> + __prerr_func("SEAMCALL (0x%llx) failed: 0x%llx\n", \
> + ((u64)__fn), ((u64)__err))
> +
> +#define SEAMCALL_REGS_FMT \
> + "RCX 0x%llx RDX 0x%llx R8 0x%llx R9 0x%llx R10 0x%llx R11 0x%llx\n"
> +
> +#define seamcall_err_ret(__fn, __err, __args, __prerr_func) \
> +({ \
> + seamcall_err((__fn), (__err), (__args), __prerr_func); \
> + __prerr_func(SEAMCALL_REGS_FMT, \
> + (__args)->rcx, (__args)->rdx, (__args)->r8, \
> + (__args)->r9, (__args)->r10, (__args)->r11); \
> +})
> +
> +#define SEAMCALL_EXTRA_REGS_FMT \
> + "RBX 0x%llx RDI 0x%llx RSI 0x%llx R12 0x%llx R13 0x%llx R14 0x%llx R15 0x%llx"
> +
> +#define seamcall_err_saved_ret(__fn, __err, __args, __prerr_func) \
> +({ \
> + seamcall_err_ret(__fn, __err, __args, __prerr_func); \
> + __prerr_func(SEAMCALL_EXTRA_REGS_FMT, \
> + (__args)->rbx, (__args)->rdi, (__args)->rsi, \
> + (__args)->r12, (__args)->r13, (__args)->r14, \
> + (__args)->r15); \
> +})
> +
> +static __always_inline bool seamcall_err_is_kernel_defined(u64 err)
> +{
> + /* All kernel defined SEAMCALL error code have TDX_SW_ERROR set */
> + return (err & TDX_SW_ERROR) == TDX_SW_ERROR;
> +}
> +
> +#define __SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func, \
> + __prerr_func) \
> +({ \
> + u64 ___sret = __seamcall_func((__fn), (__args)); \
> + \
> + /* Kernel defined error code has special meaning, leave to caller */ \
> + if (!seamcall_err_is_kernel_defined((___sret)) && \
> + ___sret != TDX_SUCCESS) \
> + __seamcall_err_func((__fn), (___sret), (__args), __prerr_func); \
> + \
> + ___sret; \
> +})
> +
> +#define SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func) \
> +({ \
> + u64 ___sret = __SEAMCALL_PRERR(__seamcall_func, __fn, __args, \
> + __seamcall_err_func, pr_err);
__SEAMCALL_PRERR seems to only ever be called with pr_err for as the
error function, can you just kill off that argument and always call pr_err.
\
> + int ___ret; \
> + \
> + switch (___sret) { \
> + case TDX_SUCCESS: \
> + ___ret = 0; \
> + break; \
> + case TDX_SEAMCALL_VMFAILINVALID: \
> + pr_err("SEAMCALL failed: TDX module not loaded.\n"); \
> + ___ret = -ENODEV; \
> + break; \
> + case TDX_SEAMCALL_GP: \
> + pr_err("SEAMCALL failed: TDX disabled by BIOS.\n"); \
> + ___ret = -EOPNOTSUPP; \
> + break; \
> + case TDX_SEAMCALL_UD: \
> + pr_err("SEAMCALL failed: CPU not in VMX operation.\n"); \
> + ___ret = -EACCES; \
> + break; \
> + default: \
> + ___ret = -EIO; \
> + } \
> + ___ret; \
> +})
> +
> +#define seamcall_prerr(__fn, __args) \
> + SEAMCALL_PRERR(seamcall, (__fn), (__args), seamcall_err)
> +
> +#define seamcall_prerr_ret(__fn, __args) \
> + SEAMCALL_PRERR(seamcall_ret, (__fn), (__args), seamcall_err_ret)
> +
> +#define seamcall_prerr_saved_ret(__fn, __args) \
> + SEAMCALL_PRERR(seamcall_saved_ret, (__fn), (__args), \
> + seamcall_err_saved_ret)
The level of indirection which you add with those seamcal_err* function
is just mind boggling:
SEAMCALL_PRERR -> __SEAMCALL_PRERR -> __seamcall_err_func ->
__prerr_func and all of this so you can have a standardized string
printing. I see no value in having __SEAMCALL_PRERR as a separate macro,
simply inline it into SEAMCALL_PRERR, replace the prerr_func argument
with a direct call to pr_err.
> +
> static u32 tdx_global_keyid __ro_after_init;
> static u32 tdx_guest_keyid_start __ro_after_init;
> static u32 tdx_nr_guest_keyids __ro_after_init;
Powered by blists - more mailing lists