diff mbox series

[RFC,Part1,07/13] x86/compressed: register GHCB memory when SNP is active

Message ID 20210324164424.28124-8-brijesh.singh@amd.com (mailing list archive)
State New, archived
Headers show
Series Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand

Commit Message

Brijesh Singh March 24, 2021, 4:44 p.m. UTC
The SEV-SNP guest is required to perform GHCB GPA registration. This is
because the hypervisor may prefer that a guest use a consistent and/or
specific GPA for the GHCB associated with a vCPU. For more information,
see the GHCB specification section 2.5.2.

Currently, we do not support working with hypervisor preferred GPA, If
the hypervisor can not work with our provided GPA then we will terminate
the boot.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 arch/x86/boot/compressed/sev-es.c  |  4 ++++
 arch/x86/boot/compressed/sev-snp.c | 26 ++++++++++++++++++++++++++
 arch/x86/include/asm/sev-snp.h     | 11 +++++++++++
 3 files changed, 41 insertions(+)

Comments

Borislav Petkov April 7, 2021, 11:59 a.m. UTC | #1
On Wed, Mar 24, 2021 at 11:44:18AM -0500, Brijesh Singh wrote:
> The SEV-SNP guest is required to perform GHCB GPA registration. This is

Why does it need to do that? Some additional security so as to not allow
changing the GHCB once it is established?

I'm guessing that's enforced by the SNP fw and we cannot do that
retroactively for SEV...? Because it sounds like a nice little thing we
could do additionally.

> because the hypervisor may prefer that a guest use a consistent and/or
> specific GPA for the GHCB associated with a vCPU. For more information,
> see the GHCB specification section 2.5.2.

I think you mean

"2.3.2 GHCB GPA Registration"

Please use the section name too because that doc changes from time to
time.

Also, you probably should update it here:

https://bugzilla.kernel.org/show_bug.cgi?id=206537

> diff --git a/arch/x86/boot/compressed/sev-snp.c b/arch/x86/boot/compressed/sev-snp.c
> index 5c25103b0df1..a4c5e85699a7 100644
> --- a/arch/x86/boot/compressed/sev-snp.c
> +++ b/arch/x86/boot/compressed/sev-snp.c
> @@ -113,3 +113,29 @@ void sev_snp_set_page_shared(unsigned long paddr)
>  {
>  	sev_snp_set_page_private_shared(paddr, SNP_PAGE_STATE_SHARED);
>  }
> +
> +void sev_snp_register_ghcb(unsigned long paddr)

Right and let's prefix SNP-specific functions with "snp_" only so that
it is clear which is wcich when looking at the code.

> +{
> +	u64 pfn = paddr >> PAGE_SHIFT;
> +	u64 old, val;
> +
> +	if (!sev_snp_enabled())
> +		return;
> +
> +	/* save the old GHCB MSR */
> +	old = sev_es_rd_ghcb_msr();
> +
> +	/* Issue VMGEXIT */

No need for that comment.

> +	sev_es_wr_ghcb_msr(GHCB_REGISTER_GPA_REQ_VAL(pfn));
> +	VMGEXIT();
> +
> +	val = sev_es_rd_ghcb_msr();
> +
> +	/* If the response GPA is not ours then abort the guest */
> +	if ((GHCB_SEV_GHCB_RESP_CODE(val) != GHCB_REGISTER_GPA_RESP) ||
> +	    (GHCB_REGISTER_GPA_RESP_VAL(val) != pfn))
> +		sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST);

Yet another example where using a specific termination reason could help
with debugging guests. Looking at the GHCB spec, I hope GHCBData[23:16]
is big enough for all reasons. I'm sure it can be extended ofc ...

:-)

> +	/* Restore the GHCB MSR value */
> +	sev_es_wr_ghcb_msr(old);
> +}
> diff --git a/arch/x86/include/asm/sev-snp.h b/arch/x86/include/asm/sev-snp.h
> index f514dad276f2..0523eb21abd7 100644
> --- a/arch/x86/include/asm/sev-snp.h
> +++ b/arch/x86/include/asm/sev-snp.h
> @@ -56,6 +56,13 @@ struct __packed snp_page_state_change {
>  	struct snp_page_state_entry entry[SNP_PAGE_STATE_CHANGE_MAX_ENTRY];
>  };
>  
> +/* GHCB GPA register */
> +#define GHCB_REGISTER_GPA_REQ	0x012UL
> +#define		GHCB_REGISTER_GPA_REQ_VAL(v)		(GHCB_REGISTER_GPA_REQ | ((v) << 12))
> +
> +#define GHCB_REGISTER_GPA_RESP	0x013UL

Let's append "UL" to the other request numbers for consistency.

Thx.
Brijesh Singh April 7, 2021, 5:34 p.m. UTC | #2
On 4/7/21 6:59 AM, Borislav Petkov wrote:
> On Wed, Mar 24, 2021 at 11:44:18AM -0500, Brijesh Singh wrote:
>> The SEV-SNP guest is required to perform GHCB GPA registration. This is
> Why does it need to do that? Some additional security so as to not allow
> changing the GHCB once it is established?
>
> I'm guessing that's enforced by the SNP fw and we cannot do that
> retroactively for SEV...? Because it sounds like a nice little thing we
> could do additionally.

The feature is part of the GHCB version 2 and is enforced by the
hypervisor. I guess it can be extended for the ES. Since this feature
was not available in GHCB version 1 (base ES) so it should be presented
as an optional for the ES ?


>
>> because the hypervisor may prefer that a guest use a consistent and/or
>> specific GPA for the GHCB associated with a vCPU. For more information,
>> see the GHCB specification section 2.5.2.
> I think you mean
>
> "2.3.2 GHCB GPA Registration"
>
> Please use the section name too because that doc changes from time to
> time.
>
> Also, you probably should update it here:
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D206537&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Ce8ae7574ecc742be6c1a08d8f9bcac94%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637533936070042328%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=NaHJ5R9Dfo%2FPnci%2B%2B6xK9ecpV0%2F%2FYbsdGl25%2BFj3TaU%3D&amp;reserved=0
>

Yes, the section may have changed since I wrote the description. Noted.
I will refer the section name.


>> diff --git a/arch/x86/boot/compressed/sev-snp.c b/arch/x86/boot/compressed/sev-snp.c
>> index 5c25103b0df1..a4c5e85699a7 100644
>> --- a/arch/x86/boot/compressed/sev-snp.c
>> +++ b/arch/x86/boot/compressed/sev-snp.c
>> @@ -113,3 +113,29 @@ void sev_snp_set_page_shared(unsigned long paddr)
>>  {
>>  	sev_snp_set_page_private_shared(paddr, SNP_PAGE_STATE_SHARED);
>>  }
>> +
>> +void sev_snp_register_ghcb(unsigned long paddr)
> Right and let's prefix SNP-specific functions with "snp_" only so that
> it is clear which is wcich when looking at the code.
>
>> +{
>> +	u64 pfn = paddr >> PAGE_SHIFT;
>> +	u64 old, val;
>> +
>> +	if (!sev_snp_enabled())
>> +		return;
>> +
>> +	/* save the old GHCB MSR */
>> +	old = sev_es_rd_ghcb_msr();
>> +
>> +	/* Issue VMGEXIT */
> No need for that comment.
>
>> +	sev_es_wr_ghcb_msr(GHCB_REGISTER_GPA_REQ_VAL(pfn));
>> +	VMGEXIT();
>> +
>> +	val = sev_es_rd_ghcb_msr();
>> +
>> +	/* If the response GPA is not ours then abort the guest */
>> +	if ((GHCB_SEV_GHCB_RESP_CODE(val) != GHCB_REGISTER_GPA_RESP) ||
>> +	    (GHCB_REGISTER_GPA_RESP_VAL(val) != pfn))
>> +		sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST);
> Yet another example where using a specific termination reason could help
> with debugging guests. Looking at the GHCB spec, I hope GHCBData[23:16]
> is big enough for all reasons. I'm sure it can be extended ofc ...


Maybe we can request the GHCB version 3 to add the extended error code.


> :-)
>
>> +	/* Restore the GHCB MSR value */
>> +	sev_es_wr_ghcb_msr(old);
>> +}
>> diff --git a/arch/x86/include/asm/sev-snp.h b/arch/x86/include/asm/sev-snp.h
>> index f514dad276f2..0523eb21abd7 100644
>> --- a/arch/x86/include/asm/sev-snp.h
>> +++ b/arch/x86/include/asm/sev-snp.h
>> @@ -56,6 +56,13 @@ struct __packed snp_page_state_change {
>>  	struct snp_page_state_entry entry[SNP_PAGE_STATE_CHANGE_MAX_ENTRY];
>>  };
>>  
>> +/* GHCB GPA register */
>> +#define GHCB_REGISTER_GPA_REQ	0x012UL
>> +#define		GHCB_REGISTER_GPA_REQ_VAL(v)		(GHCB_REGISTER_GPA_REQ | ((v) << 12))
>> +
>> +#define GHCB_REGISTER_GPA_RESP	0x013UL
> Let's append "UL" to the other request numbers for consistency.
>
> Thx.
>
Tom Lendacky April 7, 2021, 5:54 p.m. UTC | #3
On 4/7/21 12:34 PM, Brijesh Singh wrote:
> 
> On 4/7/21 6:59 AM, Borislav Petkov wrote:
>> On Wed, Mar 24, 2021 at 11:44:18AM -0500, Brijesh Singh wrote:
>>> The SEV-SNP guest is required to perform GHCB GPA registration. This is
>> Why does it need to do that? Some additional security so as to not allow
>> changing the GHCB once it is established?
>>
>> I'm guessing that's enforced by the SNP fw and we cannot do that
>> retroactively for SEV...? Because it sounds like a nice little thing we
>> could do additionally.
> 
> The feature is part of the GHCB version 2 and is enforced by the
> hypervisor. I guess it can be extended for the ES. Since this feature
> was not available in GHCB version 1 (base ES) so it should be presented
> as an optional for the ES ?

GHCB GPA registration is only supported and required for SEV-SNP guests.
The final version of the spec documents that and should be published
within the next few days.

Thanks,
Tom

> 
> 
>>
>>> because the hypervisor may prefer that a guest use a consistent and/or
>>> specific GPA for the GHCB associated with a vCPU. For more information,
>>> see the GHCB specification section 2.5.2.
>> I think you mean
>>
>> "2.3.2 GHCB GPA Registration"
>>
>> Please use the section name too because that doc changes from time to
>> time.
>>
>> Also, you probably should update it here:
>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D206537&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Ce8ae7574ecc742be6c1a08d8f9bcac94%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637533936070042328%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=NaHJ5R9Dfo%2FPnci%2B%2B6xK9ecpV0%2F%2FYbsdGl25%2BFj3TaU%3D&amp;reserved=0
>>
> 
> Yes, the section may have changed since I wrote the description. Noted.
> I will refer the section name.
> 
> 
>>> diff --git a/arch/x86/boot/compressed/sev-snp.c b/arch/x86/boot/compressed/sev-snp.c
>>> index 5c25103b0df1..a4c5e85699a7 100644
>>> --- a/arch/x86/boot/compressed/sev-snp.c
>>> +++ b/arch/x86/boot/compressed/sev-snp.c
>>> @@ -113,3 +113,29 @@ void sev_snp_set_page_shared(unsigned long paddr)
>>>  {
>>>  	sev_snp_set_page_private_shared(paddr, SNP_PAGE_STATE_SHARED);
>>>  }
>>> +
>>> +void sev_snp_register_ghcb(unsigned long paddr)
>> Right and let's prefix SNP-specific functions with "snp_" only so that
>> it is clear which is wcich when looking at the code.
>>
>>> +{
>>> +	u64 pfn = paddr >> PAGE_SHIFT;
>>> +	u64 old, val;
>>> +
>>> +	if (!sev_snp_enabled())
>>> +		return;
>>> +
>>> +	/* save the old GHCB MSR */
>>> +	old = sev_es_rd_ghcb_msr();
>>> +
>>> +	/* Issue VMGEXIT */
>> No need for that comment.
>>
>>> +	sev_es_wr_ghcb_msr(GHCB_REGISTER_GPA_REQ_VAL(pfn));
>>> +	VMGEXIT();
>>> +
>>> +	val = sev_es_rd_ghcb_msr();
>>> +
>>> +	/* If the response GPA is not ours then abort the guest */
>>> +	if ((GHCB_SEV_GHCB_RESP_CODE(val) != GHCB_REGISTER_GPA_RESP) ||
>>> +	    (GHCB_REGISTER_GPA_RESP_VAL(val) != pfn))
>>> +		sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST);
>> Yet another example where using a specific termination reason could help
>> with debugging guests. Looking at the GHCB spec, I hope GHCBData[23:16]
>> is big enough for all reasons. I'm sure it can be extended ofc ...
> 
> 
> Maybe we can request the GHCB version 3 to add the extended error code.
> 
> 
>> :-)
>>
>>> +	/* Restore the GHCB MSR value */
>>> +	sev_es_wr_ghcb_msr(old);
>>> +}
>>> diff --git a/arch/x86/include/asm/sev-snp.h b/arch/x86/include/asm/sev-snp.h
>>> index f514dad276f2..0523eb21abd7 100644
>>> --- a/arch/x86/include/asm/sev-snp.h
>>> +++ b/arch/x86/include/asm/sev-snp.h
>>> @@ -56,6 +56,13 @@ struct __packed snp_page_state_change {
>>>  	struct snp_page_state_entry entry[SNP_PAGE_STATE_CHANGE_MAX_ENTRY];
>>>  };
>>>  
>>> +/* GHCB GPA register */
>>> +#define GHCB_REGISTER_GPA_REQ	0x012UL
>>> +#define		GHCB_REGISTER_GPA_REQ_VAL(v)		(GHCB_REGISTER_GPA_REQ | ((v) << 12))
>>> +
>>> +#define GHCB_REGISTER_GPA_RESP	0x013UL
>> Let's append "UL" to the other request numbers for consistency.
>>
>> Thx.
>>
Borislav Petkov April 8, 2021, 8:17 a.m. UTC | #4
On Wed, Apr 07, 2021 at 12:34:59PM -0500, Brijesh Singh wrote:
> The feature is part of the GHCB version 2 and is enforced by the
> hypervisor. I guess it can be extended for the ES. Since this feature
> was not available in GHCB version 1 (base ES) so it should be presented
> as an optional for the ES ?

Yeah, it probably is not worth the effort. If an attacker controls the
guest kernel, then it can re-register a new GHCB so it doesn't really
matter.
diff mbox series

Patch

diff --git a/arch/x86/boot/compressed/sev-es.c b/arch/x86/boot/compressed/sev-es.c
index 58b15b7c1aa7..c85d3d9ec57a 100644
--- a/arch/x86/boot/compressed/sev-es.c
+++ b/arch/x86/boot/compressed/sev-es.c
@@ -20,6 +20,7 @@ 
 #include <asm/fpu/xcr.h>
 #include <asm/ptrace.h>
 #include <asm/svm.h>
+#include <asm/sev-snp.h>
 
 #include "error.h"
 
@@ -118,6 +119,9 @@  static bool early_setup_sev_es(void)
 	/* Initialize lookup tables for the instruction decoder */
 	inat_init_tables();
 
+	/* SEV-SNP guest requires the GHCB GPA must be registered */
+	sev_snp_register_ghcb(__pa(&boot_ghcb_page));
+
 	return true;
 }
 
diff --git a/arch/x86/boot/compressed/sev-snp.c b/arch/x86/boot/compressed/sev-snp.c
index 5c25103b0df1..a4c5e85699a7 100644
--- a/arch/x86/boot/compressed/sev-snp.c
+++ b/arch/x86/boot/compressed/sev-snp.c
@@ -113,3 +113,29 @@  void sev_snp_set_page_shared(unsigned long paddr)
 {
 	sev_snp_set_page_private_shared(paddr, SNP_PAGE_STATE_SHARED);
 }
+
+void sev_snp_register_ghcb(unsigned long paddr)
+{
+	u64 pfn = paddr >> PAGE_SHIFT;
+	u64 old, val;
+
+	if (!sev_snp_enabled())
+		return;
+
+	/* save the old GHCB MSR */
+	old = sev_es_rd_ghcb_msr();
+
+	/* Issue VMGEXIT */
+	sev_es_wr_ghcb_msr(GHCB_REGISTER_GPA_REQ_VAL(pfn));
+	VMGEXIT();
+
+	val = sev_es_rd_ghcb_msr();
+
+	/* If the response GPA is not ours then abort the guest */
+	if ((GHCB_SEV_GHCB_RESP_CODE(val) != GHCB_REGISTER_GPA_RESP) ||
+	    (GHCB_REGISTER_GPA_RESP_VAL(val) != pfn))
+		sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST);
+
+	/* Restore the GHCB MSR value */
+	sev_es_wr_ghcb_msr(old);
+}
diff --git a/arch/x86/include/asm/sev-snp.h b/arch/x86/include/asm/sev-snp.h
index f514dad276f2..0523eb21abd7 100644
--- a/arch/x86/include/asm/sev-snp.h
+++ b/arch/x86/include/asm/sev-snp.h
@@ -56,6 +56,13 @@  struct __packed snp_page_state_change {
 	struct snp_page_state_entry entry[SNP_PAGE_STATE_CHANGE_MAX_ENTRY];
 };
 
+/* GHCB GPA register */
+#define GHCB_REGISTER_GPA_REQ	0x012UL
+#define		GHCB_REGISTER_GPA_REQ_VAL(v)		(GHCB_REGISTER_GPA_REQ | ((v) << 12))
+
+#define GHCB_REGISTER_GPA_RESP	0x013UL
+#define		GHCB_REGISTER_GPA_RESP_VAL(val)		((val) >> 12)
+
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 static inline int __pvalidate(unsigned long vaddr, int rmp_psize, int validate,
 			      unsigned long *rflags)
@@ -73,6 +80,8 @@  static inline int __pvalidate(unsigned long vaddr, int rmp_psize, int validate,
 	return rc;
 }
 
+void sev_snp_register_ghcb(unsigned long paddr);
+
 #else	/* !CONFIG_AMD_MEM_ENCRYPT */
 
 static inline int __pvalidate(unsigned long vaddr, int psize, int validate, unsigned long *eflags)
@@ -80,6 +89,8 @@  static inline int __pvalidate(unsigned long vaddr, int psize, int validate, unsi
 	return 0;
 }
 
+static inline void sev_snp_register_ghcb(unsigned long paddr) { }
+
 #endif /* CONFIG_AMD_MEM_ENCRYPT */
 
 #endif	/* __ASSEMBLY__ */