| 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: <YioZnTYahkoy2Mxz@zn.tnic>
Date: Thu, 10 Mar 2022 16:30:57 +0100
From: Borislav Petkov <bp@...en8.de>
To: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Cc: tglx@...utronix.de, mingo@...hat.com, dave.hansen@...el.com,
luto@...nel.org, peterz@...radead.org,
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: [PATCHv5 03/30] x86/tdx: Add __tdx_module_call() and
__tdx_hypercall() helper functions
On Wed, Mar 02, 2022 at 05:27:39PM +0300, Kirill A. Shutemov wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>
> Guests communicate with VMMs with hypercalls. Historically, these
> are implemented using instructions that are known to cause VMEXITs
> like VMCALL, VMLAUNCH, etc. However, with TDX, VMEXITs no longer
> expose the guest state to the host. This prevents the old hypercall
> mechanisms from working. So, to communicate with VMM, TDX
> specification defines a new instruction called TDCALL.
>
> In a TDX based VM, since the VMM is an untrusted entity, an intermediary
> layer -- TDX module -- facilitates secure communication between the host
> and the guest. TDX module is loaded like a firmware into a special CPU
> mode called SEAM. TDX guests communicate with the TDX module using the
> TDCALL instruction.
>
> A guest uses TDCALL to communicate with both the TDX module and VMM.
> The value of the RAX register when executing the TDCALL instruction is
> used to determine the TDCALL type. A variant of TDCALL used to communicate
> with the VMM is called TDVMCALL.
>
> Add generic interfaces to communicate with the TDX module and VMM
> (using the TDCALL instruction).
>
> __tdx_hypercall() - Used by the guest to request services from the
> VMM (via TDVMCALL).
> __tdx_module_call() - Used to communicate with the TDX module (via
> TDCALL).
Ok, you need to fix this: this sounds to me like there are two insns:
TDCALL and TDVMCALL. But there's only TDCALL.
And I'm not even clear on how the differentiation is done - I guess
with %r11 which contains the VMCALL subfunction number in the
__tdx_hypercall() case but I'm not sure.
And when explaining this, pls put it in the comment over the function so
that it is clear how the distinction is made.
> Also define an additional wrapper _tdx_hypercall(), which adds error
> handling support for the TDCALL failure.
>
> The __tdx_module_call() and __tdx_hypercall() helper functions are
> implemented in assembly in a .S file. The TDCALL ABI requires
> shuffling arguments in and out of registers, which proved to be
> awkward with inline assembly.
>
> Just like syscalls, not all TDVMCALL use cases need to use the same
> number of argument registers. The implementation here picks the current
> worst-case scenario for TDCALL (4 registers). For TDCALLs with fewer
> than 4 arguments, there will end up being a few superfluous (cheap)
> instructions. But, this approach maximizes code reuse.
>
> For registers used by the TDCALL instruction, please check TDX GHCI
> specification, the section titled "TDCALL instruction" and "TDG.VP.VMCALL
> Interface".
>
> Based on previous patch by Sean Christopherson.
>
> Reviewed-by: Tony Luck <tony.luck@...el.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> ---
> arch/x86/coco/Makefile | 2 +-
> arch/x86/coco/tdcall.S | 188 ++++++++++++++++++++++++++++++++++
> arch/x86/coco/tdx.c | 18 ++++
Those should be
arch/x86/coco/tdx/tdcall.S
arch/x86/coco/tdx/tdx.c
like we said:
"- confidential computing guest stuff: arch/x86/coco/{sev,tdx}"
> arch/x86/include/asm/tdx.h | 27 +++++
> arch/x86/kernel/asm-offsets.c | 10 ++
> 5 files changed, 244 insertions(+), 1 deletion(-)
> create mode 100644 arch/x86/coco/tdcall.S
...
> +SYM_FUNC_START(__tdx_hypercall)
> + FRAME_BEGIN
> +
> + /* Save callee-saved GPRs as mandated by the x86_64 ABI */
> + push %r15
> + push %r14
> + push %r13
> + push %r12
> +
> + /* Mangle function call ABI into TDCALL ABI: */
> + /* Set TDCALL leaf ID (TDVMCALL (0)) in RAX */
> + xor %eax, %eax
> +
> + /* Copy hypercall registers from arg struct: */
> + movq TDX_HYPERCALL_r10(%rdi), %r10
> + movq TDX_HYPERCALL_r11(%rdi), %r11
> + movq TDX_HYPERCALL_r12(%rdi), %r12
> + movq TDX_HYPERCALL_r13(%rdi), %r13
> + movq TDX_HYPERCALL_r14(%rdi), %r14
> + movq TDX_HYPERCALL_r15(%rdi), %r15
> +
> + movl $TDVMCALL_EXPOSE_REGS_MASK, %ecx
> +
> + tdcall
> +
> + /*
> + * RAX==0 indicates a failure of the TDVMCALL mechanism itself and that
> + * something has gone horribly wrong with the TDX module.
> + *
> + * The return status of the hypercall operation is in a separate
> + * register (in R10). Hypercall errors are a part of normal operation
> + * and are handled by callers.
> + */
> + testq %rax, %rax
> + jne .Lpanic
Hm, can this call a C function which does the panic so that a proper
error message is dumped to the user so that at least she knows where the
panic comes from?
> +
> + /* TDVMCALL leaf return code is in R10 */
> + movq %r10, %rax
> +
> + /* Copy hypercall result registers to arg struct if needed */
> + testq $TDX_HCALL_HAS_OUTPUT, %rsi
> + jz .Lout
> +
> + movq %r10, TDX_HYPERCALL_r10(%rdi)
> + movq %r11, TDX_HYPERCALL_r11(%rdi)
> + movq %r12, TDX_HYPERCALL_r12(%rdi)
> + movq %r13, TDX_HYPERCALL_r13(%rdi)
> + movq %r14, TDX_HYPERCALL_r14(%rdi)
> + movq %r15, TDX_HYPERCALL_r15(%rdi)
> +.Lout:
> + /*
> + * Zero out registers exposed to the VMM to avoid speculative execution
> + * with VMM-controlled values. This needs to include all registers
> + * present in TDVMCALL_EXPOSE_REGS_MASK (except R12-R15). R12-R15
> + * context will be restored.
> + */
> + xor %r10d, %r10d
> + xor %r11d, %r11d
> +
> + /* Restore callee-saved GPRs as mandated by the x86_64 ABI */
> + pop %r12
> + pop %r13
> + pop %r14
> + pop %r15
> +
> + FRAME_END
> +
> + retq
> +.Lpanic:
> + ud2
> +SYM_FUNC_END(__tdx_hypercall)
...
> diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
> index 7dca52f5cfc6..0b465e7d0a2f 100644
> --- a/arch/x86/kernel/asm-offsets.c
> +++ b/arch/x86/kernel/asm-offsets.c
> @@ -74,6 +74,16 @@ static void __used common(void)
> OFFSET(TDX_MODULE_r10, tdx_module_output, r10);
> OFFSET(TDX_MODULE_r11, tdx_module_output, r11);
>
> +#ifdef CONFIG_INTEL_TDX_GUEST
Those have ifdeffery around them - why don't the TDX_MODULE_* ones need
it too?
> + BLANK();
> + OFFSET(TDX_HYPERCALL_r10, tdx_hypercall_args, r10);
> + OFFSET(TDX_HYPERCALL_r11, tdx_hypercall_args, r11);
> + OFFSET(TDX_HYPERCALL_r12, tdx_hypercall_args, r12);
> + OFFSET(TDX_HYPERCALL_r13, tdx_hypercall_args, r13);
> + OFFSET(TDX_HYPERCALL_r14, tdx_hypercall_args, r14);
> + OFFSET(TDX_HYPERCALL_r15, tdx_hypercall_args, r15);
> +#endif
> +
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Powered by blists - more mailing lists