| 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
| ||
|
Date: Thu, 24 Feb 2022 08:35:32 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
luto@...nel.org, peterz@...radead.org
Cc: sathyanarayanan.kuppuswamy@...ux.intel.com, aarcange@...hat.com,
ak@...ux.intel.com, dan.j.williams@...el.com, david@...hat.com,
hpa@...or.com, jgross@...e.com, jmattson@...gle.com,
joro@...tes.org, jpoimboe@...hat.com, knsathya@...nel.org,
pbonzini@...hat.com, sdeep@...are.com, seanjc@...gle.com,
tony.luck@...el.com, vkuznets@...hat.com, wanpengli@...cent.com,
thomas.lendacky@....com, brijesh.singh@....com, x86@...nel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCHv4 03/30] x86/tdx: Provide common base for SEAMCALL and
TDCALL C wrappers
...
> arch/x86/include/asm/tdx.h | 20 ++++++++
> arch/x86/kernel/asm-offsets.c | 9 ++++
> arch/x86/virt/tdxcall.S | 91 +++++++++++++++++++++++++++++++++++
> 3 files changed, 120 insertions(+)
> create mode 100644 arch/x86/virt/tdxcall.S
>
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index ba8042ce61c2..2f8cb1e53e77 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -8,6 +8,25 @@
> #define TDX_CPUID_LEAF_ID 0x21
> #define TDX_IDENT "IntelTDX "
>
> +#define TDX_SEAMCALL_VMFAILINVALID 0x8000FF00FFFF0000ULL
This needs a comment. It's a completely software-defined value, right?
> +#ifndef __ASSEMBLY__
> +
> +/*
> + * Used to gather the output registers values of the TDCALL and SEAMCALL
> + * instructions when requesting services from the TDX module.
> + *
> + * This is a software only structure and not part of the TDX module/VMM ABI.
> + */
> +struct tdx_module_output {
> + u64 rcx;
> + u64 rdx;
> + u64 r8;
> + u64 r9;
> + u64 r10;
> + u64 r11;
> +};
> +
> #ifdef CONFIG_INTEL_TDX_GUEST
>
> void __init tdx_early_init(void);
> @@ -18,4 +37,5 @@ static inline void tdx_early_init(void) { };
>
> #endif /* CONFIG_INTEL_TDX_GUEST */
>
> +#endif /* !__ASSEMBLY__ */
> #endif /* _ASM_X86_TDX_H */
> diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
> index 9fb0a2f8b62a..7dca52f5cfc6 100644
> --- a/arch/x86/kernel/asm-offsets.c
> +++ b/arch/x86/kernel/asm-offsets.c
> @@ -18,6 +18,7 @@
> #include <asm/bootparam.h>
> #include <asm/suspend.h>
> #include <asm/tlbflush.h>
> +#include <asm/tdx.h>
>
> #ifdef CONFIG_XEN
> #include <xen/interface/xen.h>
> @@ -65,6 +66,14 @@ static void __used common(void)
> OFFSET(XEN_vcpu_info_arch_cr2, vcpu_info, arch.cr2);
> #endif
>
> + BLANK();
> + OFFSET(TDX_MODULE_rcx, tdx_module_output, rcx);
> + OFFSET(TDX_MODULE_rdx, tdx_module_output, rdx);
> + OFFSET(TDX_MODULE_r8, tdx_module_output, r8);
> + OFFSET(TDX_MODULE_r9, tdx_module_output, r9);
> + OFFSET(TDX_MODULE_r10, tdx_module_output, r10);
> + OFFSET(TDX_MODULE_r11, tdx_module_output, r11);
> +
> BLANK();
> OFFSET(BP_scratch, boot_params, scratch);
> OFFSET(BP_secure_boot, boot_params, secure_boot);
> diff --git a/arch/x86/virt/tdxcall.S b/arch/x86/virt/tdxcall.S
> new file mode 100644
> index 000000000000..90569faedacc
> --- /dev/null
> +++ b/arch/x86/virt/tdxcall.S
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <asm/asm-offsets.h>
> +#include <asm/tdx.h>
> +
> +/*
> + * TDX guests use the TDCALL instruction to make requests to the
> + * TDX module and hypercalls to the VMM.
> + *
> + * TDX host user SEAMCALL instruction to make requests to TDX module.
^ uses??
> + * They are supported in Binutils >= 2.36.
> + */
While you're in there, could we make this a little more structured, please?
/*
* TDX_MODULE_CALL - common helper macro for both
* TDCALL and SEAMCALL instructions.
*
* TDCALL - used by TDX guests to make requests to the
* TDX module and hypercalls to the VMM.
* SEAMCALL - used by TDX hosts to make requests to the
* TDX module.
*
* Both instruction are supported in Binutils >= 2.36.
*/
> +#define tdcall .byte 0x66,0x0f,0x01,0xcc
> +#define seamcall .byte 0x66,0x0f,0x01,0xcf
> +
> +.macro TDX_MODULE_CALL host:req
> + /*
> + * R12 will be used as temporary storage for struct tdx_module_output
> + * pointer. Since R12-R15 registers are not used by TDCALL/SEAMCALL
> + * services supported by this function, it can be reused.
> + */
> +
> + /* Callee saved, so preserve it */
> + push %r12
> +
> + /*
> + * Push output pointer to stack.
> + * After the operation, it will be fetched into R12 register.
> + */
> + push %r9
> +
> + /* Mangle function call ABI into TDCALL/SEAMCALL ABI: */
> + /* Move Leaf ID to RAX */
> + mov %rdi, %rax
> + /* Move input 4 to R9 */
> + mov %r8, %r9
> + /* Move input 3 to R8 */
> + mov %rcx, %r8
> + /* Move input 1 to RCX */
> + mov %rsi, %rcx
> + /* Leave input param 2 in RDX */
> +
> + .if \host
> + seamcall
> + /*
> + * SEAMCALL instruction is essentially a VMExit from VMX root
> + * mode to SEAM VMX root mode. VMfailInvalid (CF=1) indicates
> + * that the targeted SEAM firmware is not loaded or disabled,
> + * or P-SEAMLDR is busy with another SEAMCALL. %rax is not
> + * changed in this case.
> + *
> + * Set %rax to TDX_SEAMCALL_VMFAILINVALID for VMfailInvalid.
> + * This value will never be used as actual SEAMCALL error code.
> + */
Why can TDX_SEAMCALL_VMFAILINVALID never be used as an actual SEAMCALL
error code? Would it be more accurate to say that today's P-SEAMLDR and
TDX module never use TDX_SEAMCALL_VMFAILINVALID as an actual SEAMCALL
error code?
There's an important distinction there. It's possible that the SEAMCALL
instruction architecture says or implies that it can never have
TDX_SEAMCALL_VMFAILINVALID as a return value. It's also possible that
this "never" comes from a guess about what P-SEAMLDRs or TDX modules can do.
> + jnc .Lno_vmfailinvalid
> + mov $TDX_SEAMCALL_VMFAILINVALID, %rax
> +.Lno_vmfailinvalid:
> + .else
> + tdcall
> + .endif
> +
> + /*
> + * Fetch output pointer from stack to R12 (It is used
> + * as temporary storage)
> + */
> + pop %r12
> +
> + /* Check for success: 0 - Successful, otherwise failed */
> + test %rax, %rax
> + jnz .Lno_output_struct
> +
> + /*
> + * Since this function can be initiated without an output pointer,
> + * check if caller provided an output struct before storing
> + * output registers.
> + */
> + test %r12, %r12
> + jz .Lno_output_struct
> +
> + /* Copy result registers to output struct: */
> + movq %rcx, TDX_MODULE_rcx(%r12)
> + movq %rdx, TDX_MODULE_rdx(%r12)
> + movq %r8, TDX_MODULE_r8(%r12)
> + movq %r9, TDX_MODULE_r9(%r12)
> + movq %r10, TDX_MODULE_r10(%r12)
> + movq %r11, TDX_MODULE_r11(%r12)
> +
> +.Lno_output_struct:
> + /* Restore the state of R12 register */
> + pop %r12
> +.endm
That doesn't look too bad. Please just send an updated version with the
revised comments.
Powered by blists - more mailing lists