| 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: <a3831d3fc926905585f9fb1e14e13e502c1f5b65.camel@intel.com>
Date: Tue, 28 Jun 2022 11:59:28 +1200
From: Kai Huang <kai.huang@...el.com>
To: Dave Hansen <dave.hansen@...el.com>, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org
Cc: seanjc@...gle.com, pbonzini@...hat.com, len.brown@...el.com,
tony.luck@...el.com, rafael.j.wysocki@...el.com,
reinette.chatre@...el.com, dan.j.williams@...el.com,
peterz@...radead.org, ak@...ux.intel.com,
kirill.shutemov@...ux.intel.com,
sathyanarayanan.kuppuswamy@...ux.intel.com,
isaku.yamahata@...el.com
Subject: Re: [PATCH v5 08/22] x86/virt/tdx: Shut down TDX module in case of
error
On Mon, 2022-06-27 at 15:56 -0700, Dave Hansen wrote:
> On 6/27/22 15:34, Kai Huang wrote:
> > On Mon, 2022-06-27 at 13:46 -0700, Dave Hansen wrote:
> > I think I can just use __always_unused for this purpose?
> >
> > So I think we put seamcall() implementation to the patch which implements
> > __seamcall(). And we can inline for seamcall() and put it in either tdx.h or
> > tdx.c, or we can use __always_unused (or the one you prefer) to get rid of the
> > warning.
> >
> > What's your opinion?
>
> A temporary __always_unused seems fine to me.
Thanks will do.
>
> > > > Alternatively, we can always add EXTABLE to TDX_MODULE_CALL macro to handle #UD
> > > > and #GP by returning dedicated error codes (please also see my reply to previous
> > > > patch for the code needed to handle), in which case we don't need such check
> > > > here.
> > > >
> > > > Always handling #UD in TDX_MODULE_CALL macro also has another advantage: there
> > > > will be no Oops for #UD regardless the issue that "there's no way to check
> > > > whether VMXON has been done" in the above comment.
> > > >
> > > > What's your opinion?
> > >
> > > I think you should explore using the EXTABLE. Let's see how it looks.
> >
> > I tried to wrote the code before. I didn't test but it should look like to
> > something below. Any comments?
> >
> > diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> > index 4b75c930fa1b..4a97ca8eb14c 100644
> > --- a/arch/x86/include/asm/tdx.h
> > +++ b/arch/x86/include/asm/tdx.h
> > @@ -8,6 +8,7 @@
> > #include <asm/ptrace.h>
> > #include <asm/shared/tdx.h>
> >
> > +#ifdef CONFIG_INTEL_TDX_HOST
> > /*
> > * SW-defined error codes.
> > *
> > @@ -18,6 +19,21 @@
> > #define TDX_SW_ERROR (TDX_ERROR | GENMASK_ULL(47, 40))
> > #define TDX_SEAMCALL_VMFAILINVALID (TDX_SW_ERROR | _UL(0xFFFF0000))
> >
> > +/*
> > + * Special error codes to indicate SEAMCALL #GP and #UD.
> > + *
> > + * SEAMCALL causes #GP when SEAMRR is not properly enabled by BIOS, and
> > + * causes #UD when CPU is not in VMX operation. Define two separate
> > + * error codes to distinguish the two cases so caller can be aware of
> > + * what caused the SEAMCALL to fail.
> > + *
> > + * Bits 61:48 are reserved bits which will never be set by the TDX
> > + * module. Borrow 2 reserved bits to represent #GP and #UD.
> > + */
> > +#define TDX_SEAMCALL_GP (TDX_ERROR | GENMASK_ULL(48, 48))
> > +#define TDX_SEAMCALL_UD (TDX_ERROR | GENMASK_ULL(49, 49))
> > +#endif
> > +
> > #ifndef __ASSEMBLY__
> >
> > /*
> > diff --git a/arch/x86/virt/vmx/tdx/tdxcall.S b/arch/x86/virt/vmx/tdx/tdxcall.S
> > index 49a54356ae99..7431c47258d9 100644
> > --- a/arch/x86/virt/vmx/tdx/tdxcall.S
> > +++ b/arch/x86/virt/vmx/tdx/tdxcall.S
> > @@ -1,6 +1,7 @@
> > /* SPDX-License-Identifier: GPL-2.0 */
> > #include <asm/asm-offsets.h>
> > #include <asm/tdx.h>
> > +#include <asm/asm.h>
> >
> > /*
> > * TDCALL and SEAMCALL are supported in Binutils >= 2.36.
> > @@ -45,6 +46,7 @@
> > /* Leave input param 2 in RDX */
> >
> > .if \host
> > +1:
> > seamcall
> > /*
> > * SEAMCALL instruction is essentially a VMExit from VMX root
> > @@ -57,9 +59,25 @@
> > * This value will never be used as actual SEAMCALL error code as
> > * it is from the Reserved status code class.
> > */
> > - jnc .Lno_vmfailinvalid
> > + jnc .Lseamcall_out
> > mov $TDX_SEAMCALL_VMFAILINVALID, %rax
> > -.Lno_vmfailinvalid:
> > + jmp .Lseamcall_out
> > +2:
> > + /*
> > + * SEAMCALL caused #GP or #UD. By reaching here %eax contains
> > + * the trap number. Check the trap number and set up the return
> > + * value to %rax.
> > + */
> > + cmp $X86_TRAP_GP, %eax
> > + je .Lseamcall_gp
> > + mov $TDX_SEAMCALL_UD, %rax
> > + jmp .Lseamcall_out
> > +.Lseamcall_gp:
> > + mov $TDX_SEAMCALL_GP, %rax
> > + jmp .Lseamcall_out
> > +
> > + _ASM_EXTABLE_FAULT(1b, 2b)
> > +.Lseamcall_out
>
> Not too bad, although the end of that is a bit ugly. It would be nicer
> if you could just return the %rax value in the exception section instead
> of having to do the transform there. Maybe have a TDX_ERROR code with
> enough bits to hold any X86_TRAP_FOO.
We already have declared bits 47:40 == 0xFF is never used by TDX module:
/*
* SW-defined error codes.
*
* Bits 47:40 == 0xFF indicate Reserved status code class that never used by
* TDX module.
*/
#define TDX_ERROR _BITUL(63)
#define TDX_SW_ERROR (TDX_ERROR | GENMASK_ULL(47, 40))
#define TDX_SEAMCALL_VMFAILINVALID (TDX_SW_ERROR | _UL(0xFFFF0000))
So how about just putting the X86_TRAP_FOO to the last 32-bits? We only have 32
traps, so 32-bits is more than enough.
#define TDX_SEAMCALL_GP (TDX_SW_ERROR | X86_TRAP_GP)
#define TDX_SEAMCALL_UD (TDX_SW_ERROR | X86_TRAP_UD)
If so, in the assembly, I think we can just XOR TDX_SW_ERROR to the %rax and
return %rax:
2:
/*
* SEAMCALL caused #GP or #UD. By reaching here %eax contains
* the trap number. Convert trap number to TDX error code by setting
* TDX_SW_ERROR to the high 32-bits of %rax.
*/
xorq $TDX_SW_ERROR, %rax
How does this look?
--
Thanks,
-Kai
Powered by blists - more mailing lists