Re: [PATCH] x86/cpu/AMD: Adjust x86_phys_bits to account for reduced PA in SEV-* guests

[Borislav Petkov]

On Wed, Mar 17, 2021 at 11:32:43AM -0700, Sean Christopherson wrote:
> Always reduce x86_phys_bits per CPUID.0x8000001f[11:6] for SEV-* guests;
> the existing flow that queries X86_FEATURE_SEV may or may not trigger
> depending on what the VMM emulates, e.g. the VMM likely does not emulate
> MSR_K8_SYSCFG.
> 
> Print a somewhat scary message and override x86_phys_bits if the VMM
> doesn't omit the C-bit from MAXPHYADDR, which can be done either by
> enumerating a lower MAXPHYADDR or by enumerating a non-zero
> PhysAddrReduction.
> 
> Failure to adjust x86_phys_bits results in a false positive for
> phys_addr_valid() if the address sets the C-bit, and may also result in
> false positives for virt_addr_valid().  This is likely benign for a well-
> functioning kernel+drivers, but it's nearly impossible to confidently
> audit all users of the *_addr_valid() helpers, so who knows.
> 
> Opportunistically force clearing of SME, SEV, and SEV_ES in this case,
> as the kernel and KVM treat those feature flags as host capabilities, not
> guest capabilities.  This is likely a nop for most deployments, e.g. KVM
> doesn't emulate MSR_K8_SYSCFG.
> 
> Note, early kernel boot code for SEV-*, e.g. get_sev_encryption_bit(),
> _requires_ the SEV feature flag to be set in CPUID in order to identify
> SEV (this requirement comes from the SEV-ES GHCB standard).  But, that
> requirement does not mean the kernel must also "advertise" SEV in its own
> CPU features array.

Sure it does - /proc/cpuinfo contains feature bits of stuff which has
been enabled in the kernel. And when it comes to SEV, yeah, that was a
lot of enablement. :-)

> 
> Fixes: d8aa7eea78a1 ("x86/mm: Add Secure Encrypted Virtualization (SEV) support")
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Joerg Roedel <joro@xxxxxxxxxx>
> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
> Cc: Brijesh Singh <brijesh.singh@xxxxxxx>
> Cc: Peter Gonda <pgonda@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> 
> Regarding clearing SME, SEV, SEV_ES, etc..., it's obviously not required,
> but to avoid false postives, identifying "SEV guest" within the kernel
> must be done with sev_active().  And if we want to display support in
> /proc/cpuinfo, IMO it should be a separate synthetic feature so that
> userspace sees "sev_guest" instead of "sev".

I'm on the fence here, frankly. We issue capabilities in the guest dmesg
in print_mem_encrypt_feature_info(). However, if someone wants to query
SEV* status in the guest, then I don't have a good suggestion where to
put it. cpuinfo is probably ok-ish, a new /sys/devices/system/cpu/caps/
or so, should work too, considering the vuln stuff we stuck there so we
can extend that. We'll see.

> 
>  arch/x86/kernel/cpu/amd.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 2d11384dc9ab..0f7f8c905226 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -15,6 +15,7 @@
>  #include <asm/cpu.h>
>  #include <asm/spec-ctrl.h>
>  #include <asm/smp.h>
> +#include <asm/mem_encrypt.h>
>  #include <asm/numa.h>
>  #include <asm/pci-direct.h>
>  #include <asm/delay.h>
> @@ -575,10 +576,33 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
>  	resctrl_cpu_detect(c);
>  }
>  
> +#define SEV_CBIT_MSG "SEV: C-bit (bit %d), overlaps MAXPHYADDR (%d bits).  VMM is buggy or malicious, overriding MAXPHYADDR to %d.\n"

Not sure about that. This will make a lot of users run scared, not
knowing what's going on and open bugzillas.

> +
>  static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
>  {
>  	u64 msr;
>  
> +	/*
> +	 * When running as an SEV guest of any flavor, update the physical
> +	 * address width to account for the C-bit and clear all of the SME/SVE
> +	 * feature flags.  As far as the kernel is concerned, the SEV flags
> +	 * enumerate what features can be used by the kernel/KVM, not what
> +	 * features have been activated by the VMM.
> +	 */
> +	if (sev_active()) {
> +		int c_bit = ilog2(sme_me_mask);
> +
> +		BUG_ON(!sme_me_mask);
> +
> +		c->x86_phys_bits -= (cpuid_ebx(0x8000001f) >> 6) & 0x3f;

Well, if that leaf is intercepted, how do you wanna trust this at all?

IOW, you have c_bit so your valid address space is [0 .. c_bit-1] no?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette