linux-kernel - Re: [PATCH v3 10/15] KVM: x86: add fields to struct kvm

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]

Message-ID: <20240314024952.w6n6ol5hjzqayn2g@amd.com>
Date: Wed, 13 Mar 2024 21:49:52 -0500
From: Michael Roth <michael.roth@....com>
To: Paolo Bonzini <pbonzini@...hat.com>
CC: <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
	<seanjc@...gle.com>, <aik@....com>, <pankaj.gupta@....com>
Subject: Re: [PATCH v3 10/15] KVM: x86: add fields to struct kvm_arch for
 CoCo features

On Mon, Feb 26, 2024 at 02:03:39PM -0500, Paolo Bonzini wrote:
> Some VM types have characteristics in common; in fact, the only use
> of VM types right now is kvm_arch_has_private_mem and it assumes that
> _all_ nonzero VM types have private memory.
> 
> We will soon introduce a VM type for SEV and SEV-ES VMs, and at that
> point we will have two special characteristics of confidential VMs
> that depend on the VM type: not just if memory is private, but
> also whether guest state is protected.  For the latter we have
> kvm->arch.guest_state_protected, which is only set on a fully initialized
> VM.
> 
> For VM types with protected guest state, we can actually fix a problem in
> the SEV-ES implementation, where ioctls to set registers do not cause an
> error even if the VM has been initialized and the guest state encrypted.
> Make sure that when using VM types that will become an error.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> Message-Id: <20240209183743.22030-7-pbonzini@...hat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  7 ++-
>  arch/x86/kvm/x86.c              | 95 +++++++++++++++++++++++++++------
>  2 files changed, 84 insertions(+), 18 deletions(-)
> 
> @@ -5552,9 +5561,13 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
>  }
>  
>  
> -static void kvm_vcpu_ioctl_x86_get_xsave2(struct kvm_vcpu *vcpu,
> -					  u8 *state, unsigned int size)
> +static int kvm_vcpu_ioctl_x86_get_xsave2(struct kvm_vcpu *vcpu,
> +					 u8 *state, unsigned int size)
>  {
> +	if (vcpu->kvm->arch.has_protected_state &&
> +	    fpstate_is_confidential(&vcpu->arch.guest_fpu))
> +		return -EINVAL;
> +
>  	/*
>  	 * Only copy state for features that are enabled for the guest.  The
>  	 * state itself isn't problematic, but setting bits in the header for
> @@ -5571,22 +5584,27 @@ static void kvm_vcpu_ioctl_x86_get_xsave2(struct kvm_vcpu *vcpu,
>  			     XFEATURE_MASK_FPSSE;
>  
>  	if (fpstate_is_confidential(&vcpu->arch.guest_fpu))
> -		return;
> +		return 0;
>  
>  	fpu_copy_guest_fpstate_to_uabi(&vcpu->arch.guest_fpu, state, size,
>  				       supported_xcr0, vcpu->arch.pkru);
> +	return 0;
>  }
>  
> -static void kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu,
> -					 struct kvm_xsave *guest_xsave)
> +static int kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu,
> +					struct kvm_xsave *guest_xsave)
>  {
> -	kvm_vcpu_ioctl_x86_get_xsave2(vcpu, (void *)guest_xsave->region,
> -				      sizeof(guest_xsave->region));
> +	return kvm_vcpu_ioctl_x86_get_xsave2(vcpu, (void *)guest_xsave->region,
> +					     sizeof(guest_xsave->region));
>  }
>  
>  static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
>  					struct kvm_xsave *guest_xsave)
>  {
> +	if (vcpu->kvm->arch.has_protected_state &&
> +	    fpstate_is_confidential(&vcpu->arch.guest_fpu))
> +		return -EINVAL;
> +
>  	if (fpstate_is_confidential(&vcpu->arch.guest_fpu))
>  		return 0;

I've been trying to get SNP running on top of these patches and hit and
issue with these due to fpstate_set_confidential() being done during
svm_vcpu_create(), so when QEMU tries to sync FPU state prior to calling
SNP_LAUNCH_FINISH it errors out. I think the same would happen with
SEV-ES as well.

Maybe fpstate_set_confidential() should be relocated to SEV_LAUNCH_FINISH
site as part of these patches?

Also, do you happen to have a pointer to the WIP QEMU patches? Happy to
help with posting/testing those since we'll need similar for
SEV_INIT2-based SNP patches.

Thanks,

Mike