linux-kernel - Re: [PATCHv2 03/29] x86/tdx: Add __tdx_module_call() and __tdx

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

Date:   Thu, 3 Feb 2022 17:44:03 +0300
From:   "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To:     Kai Huang <kai.huang@...el.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>, mingo@...hat.com,
        bp@...en8.de, 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, x86@...nel.org,
        linux-kernel@...r.kernel.org, Kai Huang <kai.huang@...ux.intel.com>
Subject: Re: [PATCHv2 03/29] x86/tdx: Add __tdx_module_call() and
 __tdx_hypercall() helper functions

On Wed, Feb 02, 2022 at 11:59:10PM +1300, Kai Huang wrote:
> 
> >  /*
> > diff --git a/arch/x86/kernel/tdxcall.S b/arch/x86/kernel/tdxcall.S
> > new file mode 100644
> > index 000000000000..ba5e8e35de36
> > --- /dev/null
> > +++ b/arch/x86/kernel/tdxcall.S
> > @@ -0,0 +1,76 @@
> > +#include <asm/asm-offsets.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.
> > + *
> > + * They 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 */
> 
> Currently host seamcall also uses a data structure which has all possible input
> registers as input argument, rather than having one argument for each register:
> 
> 	struct seamcall_regs_in {
> 		u64 rcx;
> 		u64 rdx;
> 		u64 r8;
> 		u64 r9;
> 	};
> 
> Which way is better (above struct name can be changed of course)?
> 
> Or should we rename tdx_module_output to tdx_module_regs, and use it as both
> input and output (similar to __tdx_hypercall())?

Unlike hypercall case, here we have more managable number of arguments.
I would rather keep input arguments outside of any structs. It is easier
for callers, IMO.

Any objections?

> > +
> > +	.if \host
> > +	seamcall
> > +	.else
> > +	tdcall
> > +	.endif
> > +
> > +	/*
> > +	 * Fetch output pointer from stack to R12 (It is used
> > +	 * as temporary storage)
> > +	 */
> > +	pop %r12
> 
> For host SEAMCALL, one additional check against VMfailInvalid needs to be
> done after here.  This is because at host side, both P-SEAMLDR and TDX module
> are expected to be loaded by UEFI (i.e. UEFI shell tool) before booting to the
> kernel, therefore host kernel needs to detect whether them have been loaded, by
> issuing SEAMCALL.
> 
> When SEAM software (P-SEAMLDR or TDX module) is not loaded, the SEAMCALL fails
> with VMfailInvalid.  VMfailInvalid is indicated via a combination of RFLAGS
> rather than using %rax.  In practice, RFLAGS.CF=1 can be used to check whether
> VMfailInvalid happened, and we need to have something like below:
> 
> 	/*
> 	 * 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 VMFAILINVALID for VMfailInvalid.  This value
> 	 * will never be used as actual SEAMCALL error code.
> 	 */
> 	jnb     .Lno_vmfailinvalid
> 	mov     $(VMFAILINVALID), %rax
> 	jmp     .Lno_output_struct
> 
> .Lno_vmfailinvalid:

Okay, I will add it under .if \host.

But maybe use JNC instead of JNB? Since we check for CF flag,
Jump-if-Not-Carry sounds more reasonable than Jump-if-Not-Below.
Not-Below of what?

-- 
 Kirill A. Shutemov