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]

Date: Thu, 14 Mar 2024 17:09:23 -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 Wed, Mar 13, 2024 at 09:49:52PM -0500, Michael Roth wrote:
> 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?

Talked to Tom a bit about this and that might not make much sense unless
we actually want to add some code to sync that FPU state into the VMSA
prior to encryption/measurement. Otherwise, it might as well be set to
confidential as soon as vCPU is created.

And if userspace wants to write FPU register state that will not actually
become part of the guest state, it probably does make sense to return an
error for new VM types and leave it to userspace to deal with
special-casing that vs. the other ioctls like SET_REGS/SREGS/etc.

-Mike

> 
> 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
>