[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/3] x86: Support booting under Secure Startup via SKINIT


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 26 Jan 2021 16:08:11 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=9wJ78wqJlU9I2j3LJfY52PA93FFnLin07FAcEzouDOY=; b=Eq/Fb6ZrC/6LU0cBZY1hxxhE0cDq036Nsx2c6S0Hpcx+tK8IIBPwrK6jxCv52qiThb4qsDH8MrJeVkmQpsbkydRifqjDk69QjY4PJfv/R5P6RSSDgK96SeL5De/nEMxZhJX2jPj+3UvYRStjRlJSv210qjzpW2Aqk1WSiGc5XScCnlWGon+b5ByHXo0Y0Yrf8640tBV4usrmTJXEDIKQmRjp9UVKGODup+TxlvL1ONlfROW8w0a//3WR61Myz0iBjXON0RvgREq8BqT1e6nyVErzdxmKO1zL0Jvjil5yTOVl+37LtusFALfnf17oNqy0EGknrPd1cvoBrNSuKLMsVA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ZmZTiPije0jFxVeO6DeIJt2YfNulOVrjTimJ2+tVj2X2AAGxccRh7BKPIhFbdlgLgjtHl0kwgSDNcBAEWs76aF9tJ+nQs/E7lWWYxqhedOqR+u7QrSv5SNr7tMsd/87r/OzZO2SiWiO4AbRgNiMaNEag3N1TsLtevqG2fqxvzx3RAS1rpeZIvleedoR+Adg2ondWrB/RTO6J5Tvtz+DPqFm+qf//VaA23yKyTnzHZjW9GGF71P2pAx2Raqofi3Zg/Lr39OyceQSwudK3RT0E2+epoCTQXA7Sj1hM21NFO59PIn+kcStGMPnfDXmpch4zj1E0uN7vMb0GjzeYYWZAbA==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Norbert Kamiński <norbert.kaminski@xxxxxxxxx>, Marek Kasiewicz <marek.kasiewicz@xxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>, Michal Zygowski <michal.zygowski@xxxxxxxxx>, Piotr Krol <piotr.krol@xxxxxxxxx>, Krystian Hebel <krystian.hebel@xxxxxxxxx>, "Daniel P . Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>, Rich Persaud <persaur@xxxxxxxxx>, Christopher Clark <christopher.w.clark@xxxxxxxxx>
  • Delivery-date: Tue, 26 Jan 2021 15:08:39 +0000
  • Ironport-sdr: IupRIaFd48dOdW09HPPKe/Ym7oGk5qg5cfppVQf6f8tdmHdqTCTbMjkUHBbOH5qFWktrgIrq6F MR68KCooJOxJdiXkRGTBMfKhSlXwAuRLYpJSe4T5vxOPY8YA6VWUWLn7gEYz3FiHrZNTnDw+2k m5TUgl4eAa6NC8FNFC8aWUuYFVbWDRpz8NQ/PsWiwMk05Yp++fmnxAr1QjFaCLmF0X1r4i5k1v W38xRZc1rEfQ7/LULyk5iT2vQKxKQSv8EYzPMX8IZvUXfsHnDDHwJa1Cyd1aRW7oo+CMTj8Upw q7o=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, Jan 19, 2021 at 05:23:25PM +0000, Andrew Cooper wrote:
> On 19/01/2021 15:48, Roger Pau Monné wrote:
> > On Fri, Jan 15, 2021 at 11:10:46PM +0000, Andrew Cooper wrote:
> >> From: Norbert Kamiński <norbert.kaminski@xxxxxxxxx>
> >>
> >> For now, this is simply enough logic to let Xen come up after the 
> >> bootloader
> >> has executed an SKINIT instruction to begin a Secure Startup.
> > Since I know very little about this, I might as well ask. Reading the
> > PM, SKINIT requires a payload, which is an image to boot. That image
> > must be <= 64KB and needs a special header.
> >
> > In case of booting Xen using SKINIT, what would be that payload?
> > (called SLB in the PM).
> 
> The SK/SLB is implemented by https://github.com/TrenchBoot/landing-zone/
> which does all the low level platform stuff.  It is the logical
> equivalent of the Intel-provided TXT ACM which is a blob as far as the
> world is concerned.
> 
> From a "system boot" perspective, the plan is something like this:
> 
> * Grub puts xen/kernel/initrd in memory
> * A final stanza line of "secure_launch" changes the exit of grub to be
> a DRTM DLE (Dynamic Launch Event).
> ** For Intel TXT, this is the SENTER instruction, provided that the
> firmware loaded the TXT ACM properly
> ** For AMD/Hygon, this is additionally loading landing-zone into memory,
> and using the SKINIT instruction.
> * TXT blob, or Landing Zone, do low level things.
> * They enter xen/Linux/other via a common protocol.
> 
> With this patch series in place, Xen's Multiboot2 entrypoint works just
> fine as far as "invoke the next thing" goes.  Linux has a
> work-in-progress extension to their zero-page protocol.
> 
> >> During a Secure Startup, the BSP operates with the GIF clear (blocks all
> >> external interrupts, even SMI/NMI), and INIT_REDIRECTION active (converts 
> >> INIT
> >> IPIs to #SX exceptions, if e.g. the platform needs to scrub secrets before
> >> resetting).  To afford APs the same Secure Startup protections as the BSP, 
> >> the
> >> INIT IPI must be skipped, and SIPI must be the first interrupt seen.
> >>
> >> Full details are available in AMD APM Vol2 15.27 "Secure Startup with 
> >> SKINIT"
> >>
> >> Introduce skinit_enable_intr() and call it from cpu_init(), next to the
> >> enable_nmis() which performs a related function for tboot startups.
> >>
> >> Also introduce ap_boot_method to control the sequence of actions for AP 
> >> boot.
> >>
> >> Signed-off-by: Marek Kasiewicz <marek.kasiewicz@xxxxxxxxx>
> >> Signed-off-by: Norbert Kamiński <norbert.kaminski@xxxxxxxxx>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> ---
> >> CC: Jan Beulich <JBeulich@xxxxxxxx>
> >> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >> CC: Wei Liu <wl@xxxxxxx>
> >> CC: Marek Kasiewicz <marek.kasiewicz@xxxxxxxxx>
> >> CC: Norbert Kamiński <norbert.kaminski@xxxxxxxxx>
> >> CC: Michal Zygowski <michal.zygowski@xxxxxxxxx>
> >> CC: Piotr Krol <piotr.krol@xxxxxxxx>
> >> CC: Krystian Hebel <krystian.hebel@xxxxxxxxx>
> >> CC: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> >> CC: Rich Persaud <persaur@xxxxxxxxx>
> >> CC: Christopher Clark <christopher.w.clark@xxxxxxxxx>
> >> ---
> >>  xen/arch/x86/cpu/common.c        | 32 ++++++++++++++++++++++++++++++++
> >>  xen/arch/x86/smpboot.c           | 12 +++++++++++-
> >>  xen/include/asm-x86/cpufeature.h |  1 +
> >>  xen/include/asm-x86/msr-index.h  |  1 +
> >>  xen/include/asm-x86/processor.h  |  6 ++++++
> >>  5 files changed, 51 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> >> index a684519a20..d9a103e721 100644
> >> --- a/xen/arch/x86/cpu/common.c
> >> +++ b/xen/arch/x86/cpu/common.c
> >> @@ -834,6 +834,29 @@ void load_system_tables(void)
> >>    BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));
> >>  }
> >>  
> >> +static void skinit_enable_intr(void)
> > Since this is AFAICT AMD specific code, shouldn't it better be in
> > cpu/amd.c instead of cpu/common.c?
> 
> Hygon will get sad...
> 
> It's deliberately not in AMD-specific code.

Hygon already calls into AMD specific functions in amd.c
(early_init_amd, amd_log_freq), so it wouldn't seem that weird to
place it in amd.c and share with other AMD derivatives. Likely not
worth arguing about.

> >> +{
> >> +  uint64_t val;
> >> +
> >> +  /*
> >> +   * If the platform is performing a Secure Launch via SKINIT
> >> +   * INIT_REDIRECTION flag will be active.
> >> +   */
> >> +  if ( !cpu_has_skinit || rdmsr_safe(MSR_K8_VM_CR, val) ||
> >> +       !(val & VM_CR_INIT_REDIRECTION) )
> > I would add some kind of check here to assert that APs are started in
> > the correct state, ie:
> >
> > BUG_ON(ap_boot_method == AP_BOOT_SKINIT);
> 
> At the moment, it really doesn't matter.  This change is to simply let
> Xen boot.
> 
> Later changes which do the TPM and attestation work is going to need to
> know details like this, but it will be elsewhere on boot, and one
> recovery option is going to be "please just boot and let be get back
> into the system", in which case a crosscheck is not wanted.

I still think printing a message might be helpful to know the AP has
been started in an unexpected state.

> >> +
> >> +  /*
> >> +   * We don't yet handle #SX.  Disable INIT_REDIRECTION first, before
> >> +   * enabling GIF, so a pending INIT resets us, rather than causing a
> >> +   * panic due to an unknown exception.
> >> +   */
> >> +  wrmsr_safe(MSR_K8_VM_CR, val & ~VM_CR_INIT_REDIRECTION);
> >> +  asm volatile ( ".byte 0x0f,0x01,0xdc" /* STGI */ ::: "memory" );
> > We already have one of those in smv/entry.S, so I would rather not
> > open-code the opcodes here again, but I'm not unsure whether there's a
> > suitable place for those.
> 
> I deliberately didn't.  SGTI is not something we should have a helper
> for - it's not safe for general use.

Oh, I wasn't thinking of a helper, but rather a define for the
open-coded .byte directive (ie: like the one in svm/entry.S) that's
shared between both users. No big deal anyway.

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.