diff mbox series

[v2,11/14] x86/alt: Adjust _alternative_instructions() to not create shadow stacks

Message ID 20200527191847.17207-12-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86: Support for CET Supervisor Shadow Stacks | expand

Commit Message

Andrew Cooper May 27, 2020, 7:18 p.m. UTC 
The current alternatives algorithm clears CR0.WP and writes into .text.  This
has a side effect of the mappings becoming shadow stacks once CET is active.

Adjust _alternative_instructions() to clean up after itself.  This involves
extending the set of bits modify_xen_mappings() to include Dirty (and Accessed
for good measure).

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/alternative.c | 14 ++++++++++++++
 xen/arch/x86/mm.c          |  6 +++---
 2 files changed, 17 insertions(+), 3 deletions(-)

Comments

Jan Beulich May 29, 2020, 12:23 p.m. UTC | #1 
On 27.05.2020 21:18, Andrew Cooper wrote:
> @@ -398,6 +399,19 @@ static void __init _alternative_instructions(bool force)
>          panic("Timed out waiting for alternatives self-NMI to hit\n");
>  
>      set_nmi_callback(saved_nmi_callback);
> +
> +    /*
> +     * When Xen is using shadow stacks, the alternatives clearing CR0.WP and
> +     * writing into the mappings set dirty bits, turning the mappings into
> +     * shadow stack mappings.
> +     *
> +     * While we can execute from them, this would also permit them to be the
> +     * target of WRSS instructions, so reset the dirty after patching.
> +     */
> +    if ( cpu_has_xen_shstk )
> +        modify_xen_mappings(XEN_VIRT_START + MB(2),
> +                            (unsigned long)&__2M_text_end,
> +                            PAGE_HYPERVISOR_RX);

Am I misremembering, or did you post a patch before that should
be part of this series, as being a prereq to this change,
making modify_xen_mappings() also respect _PAGE_DIRTY as
requested by the caller?

Additionally I notice this

        if ( desc->Attribute & (efi_bs_revision < EFI_REVISION(2, 5)
                                ? EFI_MEMORY_WP : EFI_MEMORY_RO) )
            prot &= ~_PAGE_RW;

in efi_init_memory(). Afaict we will need to clear _PAGE_DIRTY
there as well, with prot starting out as PAGE_HYPERVISOR_RWX.

Jan
Andrew Cooper May 29, 2020, 7:46 p.m. UTC | #2 
On 29/05/2020 13:23, Jan Beulich wrote:
> On 27.05.2020 21:18, Andrew Cooper wrote:
>> @@ -398,6 +399,19 @@ static void __init _alternative_instructions(bool force)
>>          panic("Timed out waiting for alternatives self-NMI to hit\n");
>>  
>>      set_nmi_callback(saved_nmi_callback);
>> +
>> +    /*
>> +     * When Xen is using shadow stacks, the alternatives clearing CR0.WP and
>> +     * writing into the mappings set dirty bits, turning the mappings into
>> +     * shadow stack mappings.
>> +     *
>> +     * While we can execute from them, this would also permit them to be the
>> +     * target of WRSS instructions, so reset the dirty after patching.
>> +     */
>> +    if ( cpu_has_xen_shstk )
>> +        modify_xen_mappings(XEN_VIRT_START + MB(2),
>> +                            (unsigned long)&__2M_text_end,
>> +                            PAGE_HYPERVISOR_RX);
> Am I misremembering, or did you post a patch before that should
> be part of this series, as being a prereq to this change,
> making modify_xen_mappings() also respect _PAGE_DIRTY as
> requested by the caller?

No.  Its the hunk you deleted from lower in this patch.

> Additionally I notice this
>
>         if ( desc->Attribute & (efi_bs_revision < EFI_REVISION(2, 5)
>                                 ? EFI_MEMORY_WP : EFI_MEMORY_RO) )
>             prot &= ~_PAGE_RW;
>
> in efi_init_memory(). Afaict we will need to clear _PAGE_DIRTY
> there as well, with prot starting out as PAGE_HYPERVISOR_RWX.

Ok.  I'll pull that out into a separate patch.

~Andrew
diff mbox series

Patch

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index ce2b4302e6..004e9ede25 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -21,6 +21,7 @@ 
 #include <asm/processor.h>
 #include <asm/alternative.h>
 #include <xen/init.h>
+#include <asm/setup.h>
 #include <asm/system.h>
 #include <asm/traps.h>
 #include <asm/nmi.h>
@@ -398,6 +399,19 @@  static void __init _alternative_instructions(bool force)
         panic("Timed out waiting for alternatives self-NMI to hit\n");
 
     set_nmi_callback(saved_nmi_callback);
+
+    /*
+     * When Xen is using shadow stacks, the alternatives clearing CR0.WP and
+     * writing into the mappings set dirty bits, turning the mappings into
+     * shadow stack mappings.
+     *
+     * While we can execute from them, this would also permit them to be the
+     * target of WRSS instructions, so reset the dirty after patching.
+     */
+    if ( cpu_has_xen_shstk )
+        modify_xen_mappings(XEN_VIRT_START + MB(2),
+                            (unsigned long)&__2M_text_end,
+                            PAGE_HYPERVISOR_RX);
 }
 
 void __init alternative_instructions(void)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 4d6d22cc41..711b12828f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5442,8 +5442,8 @@  int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
  * mappings, but will shatter superpages if necessary, and will destroy
  * mappings if not passed _PAGE_PRESENT.
  *
- * The only flags considered are NX, RW and PRESENT.  All other input flags
- * are ignored.
+ * The only flags considered are NX, D, A, RW and PRESENT.  All other input
+ * flags are ignored.
  *
  * It is an error to call with present flags over an unpopulated range.
  */
@@ -5456,7 +5456,7 @@  int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
     unsigned long v = s;
 
     /* Set of valid PTE bits which may be altered. */
-#define FLAGS_MASK (_PAGE_NX|_PAGE_RW|_PAGE_PRESENT)
+#define FLAGS_MASK (_PAGE_NX|_PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_RW|_PAGE_PRESENT)
     nf &= FLAGS_MASK;
 
     ASSERT(IS_ALIGNED(s, PAGE_SIZE));