| Message ID | 20200428151725.31091-25-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: SEV-ES Guest Support | expand |
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
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/
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.
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
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)