[v3,24/75] x86/boot/compressed/64: Unmap GHCB page before booting the kernel
diff mbox series

Message ID 20200428151725.31091-25-joro@8bytes.org
State New
Headers show
Series
  • x86: SEV-ES Guest Support
Related show

Commit Message

Joerg Roedel April 28, 2020, 3:16 p.m. UTC
From: Joerg Roedel <jroedel@suse.de>

Force a page-fault on any further accesses to the GHCB page when they
shouldn't happen anymore. This will catch the bugs where a #VC exception
is raised when no one is expected anymore.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/boot/compressed/ident_map_64.c | 23 +++++++++++++++++++----
 arch/x86/boot/compressed/misc.h         |  6 ++++++
 arch/x86/boot/compressed/sev-es.c       | 14 ++++++++++++++
 3 files changed, 39 insertions(+), 4 deletions(-)

Comments

Borislav Petkov May 13, 2020, 11:13 a.m. UTC | #1
On Tue, Apr 28, 2020 at 05:16:34PM +0200, Joerg Roedel wrote:
> @@ -302,9 +313,13 @@ void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
>  	 *	- User faults
>  	 *	- Reserved bits set
>  	 */
> -	if (error_code & (X86_PF_PROT | X86_PF_USER | X86_PF_RSVD)) {
> +	if (ghcb_fault ||
> +	    error_code & (X86_PF_PROT | X86_PF_USER | X86_PF_RSVD)) {
>  		/* Print some information for debugging */
> -		error_putstr("Unexpected page-fault:");
> +		if (ghcb_fault)
> +			error_putstr("Page-fault on GHCB page:");
> +		else
> +			error_putstr("Unexpected page-fault:");

You could carve out the info dumping into a separate function to
unclutter this if-statement (diff ontop):

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index d3771d455249..c1979fc0f853 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -296,6 +296,22 @@ int set_page_non_present(unsigned long address)
 	return set_clr_page_flags(&mapping_info, address, 0, _PAGE_PRESENT);
 }
 
+static void do_pf_error(const char *msg, unsigned long error_code,
+			unsigned long address, unsigned long ip)
+{
+	error_putstr(msg);
+
+	error_putstr("\nError Code: ");
+	error_puthex(error_code);
+	error_putstr("\nCR2: 0x");
+	error_puthex(address);
+	error_putstr("\nRIP relative to _head: 0x");
+	error_puthex(ip - (unsigned long)_head);
+	error_putstr("\n");
+
+	error("Stopping.\n");
+}
+
 void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
 {
 	unsigned long address = native_read_cr2();
@@ -309,27 +325,15 @@ void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
 
 	/*
 	 * Check for unexpected error codes. Unexpected are:
+	 *	- Faults on the GHCB page due to unexpected #VCs
 	 *	- Faults on present pages
 	 *	- User faults
 	 *	- Reserved bits set
 	 */
-	if (ghcb_fault ||
-	    error_code & (X86_PF_PROT | X86_PF_USER | X86_PF_RSVD)) {
-		/* Print some information for debugging */
-		if (ghcb_fault)
-			error_putstr("Page-fault on GHCB page:");
-		else
-			error_putstr("Unexpected page-fault:");
-		error_putstr("\nError Code: ");
-		error_puthex(error_code);
-		error_putstr("\nCR2: 0x");
-		error_puthex(address);
-		error_putstr("\nRIP relative to _head: 0x");
-		error_puthex(regs->ip - (unsigned long)_head);
-		error_putstr("\n");
-
-		error("Stopping.\n");
-	}
+	if (ghcb_fault)
+		do_pf_error("Page-fault on GHCB page:", error_code, address, regs->ip);
+	else if (error_code & (X86_PF_PROT | X86_PF_USER | X86_PF_RSVD))
+		do_pf_error("Unexpected page-fault:", error_code, address, regs->ip);
 
 	/*
 	 * Error code is sane - now identity map the 2M region around
Joerg Roedel May 13, 2020, 11:30 a.m. UTC | #2
On Wed, May 13, 2020 at 01:13:40PM +0200, Borislav Petkov wrote:
> On Tue, Apr 28, 2020 at 05:16:34PM +0200, Joerg Roedel wrote:
> > @@ -302,9 +313,13 @@ void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
> >  	 *	- User faults
> >  	 *	- Reserved bits set
> >  	 */
> > -	if (error_code & (X86_PF_PROT | X86_PF_USER | X86_PF_RSVD)) {
> > +	if (ghcb_fault ||
> > +	    error_code & (X86_PF_PROT | X86_PF_USER | X86_PF_RSVD)) {
> >  		/* Print some information for debugging */
> > -		error_putstr("Unexpected page-fault:");
> > +		if (ghcb_fault)
> > +			error_putstr("Page-fault on GHCB page:");
> > +		else
> > +			error_putstr("Unexpected page-fault:");
> 
> You could carve out the info dumping into a separate function to
> unclutter this if-statement (diff ontop):

Yeah, I had this this way in v2, but changed it upon you request[1] :)


	Joerg

[1] https://lore.kernel.org/lkml/20200402114941.GA9352@zn.tnic/
Borislav Petkov May 13, 2020, 11:46 a.m. UTC | #3
On Wed, May 13, 2020 at 01:30:11PM +0200, Joerg Roedel wrote:
> Yeah, I had this this way in v2, but changed it upon you request[1] :)

Yeah, I was wondering why this isn't a separate function - you like them
so much. :-P

> [1] https://lore.kernel.org/lkml/20200402114941.GA9352@zn.tnic/

But that one didn't have the ghcb_fault check. Maybe it was being added
later... :)

Thx.
Joerg Roedel June 3, 2020, 10:40 a.m. UTC | #4
On Wed, May 13, 2020 at 01:46:33PM +0200, Borislav Petkov wrote:
> On Wed, May 13, 2020 at 01:30:11PM +0200, Joerg Roedel wrote:
> > Yeah, I had this this way in v2, but changed it upon you request[1] :)
> 
> Yeah, I was wondering why this isn't a separate function - you like them
> so much. :-P
> 
> > [1] https://lore.kernel.org/lkml/20200402114941.GA9352@zn.tnic/
> 
> But that one didn't have the ghcb_fault check. Maybe it was being added
> later... :)

Yes, it was :)

I changed it back, first in the patch adding the page-fault handler and
also updated this patch.


	Joerg

Patch
diff mbox series

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index bb68e9c9d87a..d3771d455249 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -291,10 +291,21 @@  int set_page_encrypted(unsigned long address)
 	return set_clr_page_flags(&mapping_info, address, _PAGE_ENC, 0);
 }
 
+int set_page_non_present(unsigned long address)
+{
+	return set_clr_page_flags(&mapping_info, address, 0, _PAGE_PRESENT);
+}
+
 void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
 {
-	unsigned long address = native_read_cr2() & PMD_MASK;
-	unsigned long end = address + PMD_SIZE;
+	unsigned long address = native_read_cr2();
+	unsigned long end;
+	bool ghcb_fault;
+
+	ghcb_fault = sev_es_check_ghcb_fault(address);
+
+	address   &= PMD_MASK;
+	end        = address + PMD_SIZE;
 
 	/*
 	 * Check for unexpected error codes. Unexpected are:
@@ -302,9 +313,13 @@  void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
 	 *	- User faults
 	 *	- Reserved bits set
 	 */
-	if (error_code & (X86_PF_PROT | X86_PF_USER | X86_PF_RSVD)) {
+	if (ghcb_fault ||
+	    error_code & (X86_PF_PROT | X86_PF_USER | X86_PF_RSVD)) {
 		/* Print some information for debugging */
-		error_putstr("Unexpected page-fault:");
+		if (ghcb_fault)
+			error_putstr("Page-fault on GHCB page:");
+		else
+			error_putstr("Unexpected page-fault:");
 		error_putstr("\nError Code: ");
 		error_puthex(error_code);
 		error_putstr("\nCR2: 0x");
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 4d37a28370ed..2e5f82acc122 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -100,6 +100,7 @@  static inline void choose_random_location(unsigned long input,
 #ifdef CONFIG_X86_64
 extern int set_page_decrypted(unsigned long address);
 extern int set_page_encrypted(unsigned long address);
+extern int set_page_non_present(unsigned long address);
 extern unsigned char _pgtable[];
 #endif
 
@@ -117,8 +118,13 @@  void set_sev_encryption_mask(void);
 
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 void sev_es_shutdown_ghcb(void);
+extern bool sev_es_check_ghcb_fault(unsigned long address);
 #else
 static inline void sev_es_shutdown_ghcb(void) { }
+static inline bool sev_es_check_ghcb_fault(unsigned long address)
+{
+	return false;
+}
 #endif
 
 /* acpi.c */
diff --git a/arch/x86/boot/compressed/sev-es.c b/arch/x86/boot/compressed/sev-es.c
index 940d72571fc9..1241697dd156 100644
--- a/arch/x86/boot/compressed/sev-es.c
+++ b/arch/x86/boot/compressed/sev-es.c
@@ -120,6 +120,20 @@  void sev_es_shutdown_ghcb(void)
 	 */
 	if (set_page_encrypted((unsigned long)&boot_ghcb_page))
 		error("Can't map GHCB page encrypted");
+
+	/*
+	 * GHCB page is mapped encrypted again and flushed from the cache.
+	 * Mark it non-present now to catch bugs when #VC exceptions trigger
+	 * after this point.
+	 */
+	if (set_page_non_present((unsigned long)&boot_ghcb_page))
+		error("Can't unmap GHCB page");
+}
+
+bool sev_es_check_ghcb_fault(unsigned long address)
+{
+	/* Check whether the fault was on the GHCB page */
+	return ((address & PAGE_MASK) == (unsigned long)&boot_ghcb_page);
 }
 
 void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)