Re: [PATCH v12 22/46] x86/sev: Use SEV-SNP AP creation to start secondary CPUs

[Brijesh Singh]

Hi Sean,

On 4/4/22 19:24, Sean Christopherson wrote:

> > +static void snp_cleanup_vmsa(struct sev_es_save_area *vmsa)
> > +{
> > +	int err;
> > +
> > +	err = snp_set_vmsa(vmsa, false);
>
> Uh, so what happens if a malicious guest does RMPADJUST to convert a VMSA page
> back to a "normal" page while the host is trying to VMRUN that VMSA?  Does VMRUN
> fault?

When SEV-SNP is enabled, the VMRUN instruction performs an additional 
security checks on various memory pages. In the case of VMSA page, 
hardware enforce that page is marked as "VMSA" in the RMP table. If not, 
 VMRUN will fail with VMEXIT_INVALID.

After the VMRUN is successful, the VMSA page is marked IN_USE by the 
hardware, any attempt to modify the RMP entries will result in 
FAIL_INUSE error. The IN_USE marking is automatically cleared by the 
hardware after the #VMEXIT.

Please see the APM vol2 section 15.36.12 for additional information.

> Can Linux refuse to support this madness and instead require the ACPI MP wakeup
> protocol being proposed/implemented for TDX?  That would allow KVM to have at

My two cents

In the current architecture, the HV track VMSAs by their SPA and guest 
controls when they are runnable. It provides flexibility to the guest, 
which can add and remove the VMSA. This flexibility may come in handy to 
support the kexec and reboot use cases. The current approach does not 
depend on ACPI; it will also come in handy to support microvm 
(minimalist machine type without PCI nor ACPI support).


> least a chance of refusing to support AP "creation", which IMO is a CVE or three
> waiting to happen.  From a KVM perspective, I don't ever want to be running a
> guest-defined VMSA.
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2FYWnbfCet84Vup6q9%40google.com&data=04%7C01%7Cbrijesh.singh%40amd.com%7Ce6a0199ed3344529241208da169ab52b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637847150997306218%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=w1eo3vza4Txv6tcgB6aO1rCoYOygQvGwKZ1kajgCbpY%3D&reserved=0
>
> > +	if (err)
> > +		pr_err("clear VMSA page failed (%u), leaking page\n", err);
> > +	else
> > +		free_page((unsigned long)vmsa);