Re: SVSM Attestation and vTPM specification additions - v0.61

[Tom Lendacky <thomas.lendacky@amd.com>]

On 2/24/23 08:15, Jörg Rödel wrote:
> On Tue, Feb 21, 2023 at 04:07:46PM -0600, Tom Lendacky wrote:
>> VCPU create doesn't actually replace the VMSA from the hypervisor point of
>> view. The guest is still required to invoke the GHCB interface to notify the
>> hypervisor.
>>
>> The SVSM PoC leaves any current VMSA in the list of all VMSAs, but does
>> replace the current / in-use version of the VMSA for the specified VCPU.
>> This is used to examine the register associated with a request, so the guest
>> can shoot itself in the foot if it doesn't switch that VCPU to the new VMSA.
>>
>> I'm ok with either doing that or, as you suggest below, checking if there's
>> an existing VMSA for the VMPL level and failing the request until the the
>> existing VMSA is deleted. That does require the deletion/creation to be
>> offloaded to another VCPU, though (unless we allow the self-deletion change
>> talked about below).
> 
> Yeah, so the current protocol expects a bit too much from the code
> running at VMPL1 for my personal taste. I think it would be better if we
> put some constraints into the handling of CREATE_VCPU and DELETE_VCPU
> calls to keep the number of pages marked as VMSA as small as possible.
> 
> With the current semantics the guest is required to call DELETE_VCPU on
> any VMSA it no longer uses. But it is not required to do so. Also there
> is a race window between switching to the new VMSA and deleting the old
> one where the HV could run the old and the new VMSA in parallel.
> 
> The SVSM will not get to see any old VMSA before it is retired (e.g. to
> clear EFER.SVME and make it invalid) until the DELETE_VCPU call, but at
> that time the new VMSA could already be running.

We could specify that any vCPU create where there's an existing VMSA at 
the VMPL level specified, results in the existing VMSA being "deleted" (an 
implicit call to delete vCPU) before performing the create vCPU. If the 
old VMSA is executing, this would prevent the creation until the old VMSA 
can be deleted.

The guest could get itself in trouble no matter what if it does not 
quiesce the target vCPU before either calling delete or create vCPU. So I 
think the above would work ok.

> 
> I don't think we can fully prevent such attacks, but we can make them
> more complicated by having some constraints who and when VCPUs can be
> created and deleted.
> 
> In particular:
> 
> 	1) VCPUs can only delete themselves. Exception is VCPU 0
> 	   (or whichever VCPU the BSP is).

I don't think we should have this limitation. We don't have the limitation 
today when the kernel is running at VMPL0, so not sure we should have it 
here. If the guest wants to do it, then it is really up to them as I don't 
think it's an attack against the SVSM, only against itself.

> 
> 	2) VCPUs can only be created for same or higher VMPLs on any
> 	   VCPU in the guest, given that there is no VMSA configured for
> 	   this (VCPU, VMPL) combination.

Agreed, we need to verify that the caller is not trying to create a VMSA 
at a higher VMPL privilege level that it is currently running at.

> 
> These are constraints the SVSM can enforce and it should keep the number
> of VMSAs in the system minimal.
> 
> It requires some changes to the current OVMF patches for SNP and SVSM,
> though.
> 
>> It might be desirable to allow another APIC to delete a VMSA. Maybe kexec
>> processing would be able to benefit from that.
> 
> Yeah, maybe. On the other side kexec would benefit if VCPUs delete
> themselves in the old kernel. Otherwise the new kernel needs to find the
> VMSA GPAs und delete them manually.
> 
> There is already a code-path for kexec/kdump where the non-boot CPUs are
> parked. This could be instrumented. If a VCPU deletes itself the SVSM
> will go into HLT until a new VCPU is created.

Yes, this would need to be documented in the spec to be sure we get the 
proper behavior on a self deletion.

Thanks,
Tom

> 
>> Should the clearing of the SVME bit fail, then a FAIL_INUSE return code
>> would be returned. I wouldn't think that the synchronization would be that
>> difficult, but I'm not sure. If we do allow self-deletion, then, I agree,
>> that would work nicely.
> 
> The question here is, can we reliably detect within the SVSM that
> clearing EFER.SVME failed?
> 
> Regards,
>