[v2,for-4.14] x86/livepatch: Make livepatching compatible with CET Shadow Stacks
diff mbox series

Message ID 20200626122408.19151-1-andrew.cooper3@citrix.com
State New
Headers show
Series
  • [v2,for-4.14] x86/livepatch: Make livepatching compatible with CET Shadow Stacks
Related show

Commit Message

Andrew Cooper June 26, 2020, 12:24 p.m. UTC
Just like the alternatives infrastructure, the livepatch infrastructure
disables CR0.WP to perform patching, which is not permitted with CET active.

Modify arch_livepatch_{quiesce,revive}() to disable CET before disabling WP,
and reset the dirty bits on all virtual regions before re-enabling CET.

One complication is that arch_livepatch_revive() has to fix up the top of the
shadow stack.  This depends on the functions not being inlined, even under
LTO.  Another limitation is that reset_virtual_region_perms() may shatter the
final superpage of .text depending on alignment.

This logic, and its downsides, are temporary until the patching infrastructure
can be adjusted to not use CR0.WP.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
CC: Pawel Wieczorkiewicz <wipawel@amazon.de>
CC: Paul Durrant <paul@xen.org>

For 4.14.  This is a bug in a 4.14 feature, with a very low risk to non-CET
usecases.

v2:
 * nolinline, and extra ifdefary
 * Expand comments
---
 xen/arch/x86/livepatch.c         | 35 +++++++++++++++++++++++++++++++++--
 xen/common/virtual_region.c      | 15 +++++++++++++++
 xen/include/xen/virtual_region.h |  1 +
 3 files changed, 49 insertions(+), 2 deletions(-)

Comments

Jan Beulich June 26, 2020, 1:14 p.m. UTC | #1
On 26.06.2020 14:24, Andrew Cooper wrote:
> Just like the alternatives infrastructure, the livepatch infrastructure
> disables CR0.WP to perform patching, which is not permitted with CET active.
> 
> Modify arch_livepatch_{quiesce,revive}() to disable CET before disabling WP,
> and reset the dirty bits on all virtual regions before re-enabling CET.
> 
> One complication is that arch_livepatch_revive() has to fix up the top of the
> shadow stack.  This depends on the functions not being inlined, even under
> LTO.  Another limitation is that reset_virtual_region_perms() may shatter the
> final superpage of .text depending on alignment.
> 
> This logic, and its downsides, are temporary until the patching infrastructure
> can be adjusted to not use CR0.WP.

In particular on this basis ...

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Paul Durrant June 26, 2020, 1:21 p.m. UTC | #2
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 26 June 2020 14:15
> To: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Xen-devel <xen-devel@lists.xenproject.org>; Wei Liu <wl@xen.org>; Roger Pau Monné
> <roger.pau@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Ross Lagerwall
> <ross.lagerwall@citrix.com>; Pawel Wieczorkiewicz <wipawel@amazon.de>; Paul Durrant <paul@xen.org>
> Subject: Re: [PATCH v2 for-4.14] x86/livepatch: Make livepatching compatible with CET Shadow Stacks
> 
> On 26.06.2020 14:24, Andrew Cooper wrote:
> > Just like the alternatives infrastructure, the livepatch infrastructure
> > disables CR0.WP to perform patching, which is not permitted with CET active.
> >
> > Modify arch_livepatch_{quiesce,revive}() to disable CET before disabling WP,
> > and reset the dirty bits on all virtual regions before re-enabling CET.
> >
> > One complication is that arch_livepatch_revive() has to fix up the top of the
> > shadow stack.  This depends on the functions not being inlined, even under
> > LTO.  Another limitation is that reset_virtual_region_perms() may shatter the
> > final superpage of .text depending on alignment.
> >
> > This logic, and its downsides, are temporary until the patching infrastructure
> > can be adjusted to not use CR0.WP.
> 
> In particular on this basis ...
> 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Release-acked-by: Paul Durrant <paul@xen.org>

> 
> Jan
Ross Lagerwall June 26, 2020, 1:59 p.m. UTC | #3
On 2020-06-26 13:24, Andrew Cooper wrote:
> Just like the alternatives infrastructure, the livepatch infrastructure
> disables CR0.WP to perform patching, which is not permitted with CET active.
> 
> Modify arch_livepatch_{quiesce,revive}() to disable CET before disabling WP,
> and reset the dirty bits on all virtual regions before re-enabling CET.
> 
> One complication is that arch_livepatch_revive() has to fix up the top of the
> shadow stack.  This depends on the functions not being inlined, even under
> LTO.  Another limitation is that reset_virtual_region_perms() may shatter the
> final superpage of .text depending on alignment.
> 
> This logic, and its downsides, are temporary until the patching infrastructure
> can be adjusted to not use CR0.WP.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> CC: Pawel Wieczorkiewicz <wipawel@amazon.de>
> CC: Paul Durrant <paul@xen.org>
> 
> For 4.14.  This is a bug in a 4.14 feature, with a very low risk to non-CET
> usecases.
> 
> v2:
>  * nolinline, and extra ifdefary
>  * Expand comments
> ---
>  xen/arch/x86/livepatch.c         | 35 +++++++++++++++++++++++++++++++++--
>  xen/common/virtual_region.c      | 15 +++++++++++++++
>  xen/include/xen/virtual_region.h |  1 +
>  3 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index 901fad96bf..49f0d902e5 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -12,6 +12,7 @@
>  #include <xen/livepatch.h>
>  #include <xen/sched.h>
>  #include <xen/vm_event.h>
> +#include <xen/virtual_region.h>
>  
>  #include <asm/fixmap.h>
>  #include <asm/nmi.h>
> @@ -56,18 +57,48 @@ int arch_livepatch_safety_check(void)
>      return -EBUSY;
>  }
>  
> -int arch_livepatch_quiesce(void)
> +int noinline arch_livepatch_quiesce(void)
>  {
> +    /* If Shadow Stacks are in use, disable CR4.CET so we can modify CR0.WP. */
> +    if ( cpu_has_xen_shstk )

Should this be:
    if ( IS_ENABLED(CONFIG_XEN_SHSTK) && cpu_has_xen_shstk )

to match arch_livepatch_revive?

Ross
Jan Beulich June 26, 2020, 2:26 p.m. UTC | #4
On 26.06.2020 15:59, Ross Lagerwall wrote:
> On 2020-06-26 13:24, Andrew Cooper wrote:
>> @@ -56,18 +57,48 @@ int arch_livepatch_safety_check(void)
>>      return -EBUSY;
>>  }
>>  
>> -int arch_livepatch_quiesce(void)
>> +int noinline arch_livepatch_quiesce(void)
>>  {
>> +    /* If Shadow Stacks are in use, disable CR4.CET so we can modify CR0.WP. */
>> +    if ( cpu_has_xen_shstk )
> 
> Should this be:
>     if ( IS_ENABLED(CONFIG_XEN_SHSTK) && cpu_has_xen_shstk )
> 
> to match arch_livepatch_revive?

While it may look a little asymmetric, I think it's preferable
to is IS_ENABLED() only where really needed, i.e. here it
guarding code that otherwise may not build.

Jan
Andrew Cooper June 26, 2020, 2:46 p.m. UTC | #5
On 26/06/2020 15:26, Jan Beulich wrote:
> On 26.06.2020 15:59, Ross Lagerwall wrote:
>> On 2020-06-26 13:24, Andrew Cooper wrote:
>>> @@ -56,18 +57,48 @@ int arch_livepatch_safety_check(void)
>>>      return -EBUSY;
>>>  }
>>>  
>>> -int arch_livepatch_quiesce(void)
>>> +int noinline arch_livepatch_quiesce(void)
>>>  {
>>> +    /* If Shadow Stacks are in use, disable CR4.CET so we can modify CR0.WP. */
>>> +    if ( cpu_has_xen_shstk )
>> Should this be:
>>     if ( IS_ENABLED(CONFIG_XEN_SHSTK) && cpu_has_xen_shstk )
>>
>> to match arch_livepatch_revive?
> While it may look a little asymmetric, I think it's preferable
> to is IS_ENABLED() only where really needed, i.e. here it
> guarding code that otherwise may not build.

The reason for the asymmetry is because of the asm() block, which needs
compiling out when we detect that we don't have a compatible assembler.

I was wondering whether I should make cpu_has_xen_shstk be false for
!CONFIG_XEN_SHSTK, but that would be 4.15 work at this point.

~Andrew
Jan Beulich June 26, 2020, 2:49 p.m. UTC | #6
On 26.06.2020 16:46, Andrew Cooper wrote:
> On 26/06/2020 15:26, Jan Beulich wrote:
>> On 26.06.2020 15:59, Ross Lagerwall wrote:
>>> On 2020-06-26 13:24, Andrew Cooper wrote:
>>>> @@ -56,18 +57,48 @@ int arch_livepatch_safety_check(void)
>>>>      return -EBUSY;
>>>>  }
>>>>  
>>>> -int arch_livepatch_quiesce(void)
>>>> +int noinline arch_livepatch_quiesce(void)
>>>>  {
>>>> +    /* If Shadow Stacks are in use, disable CR4.CET so we can modify CR0.WP. */
>>>> +    if ( cpu_has_xen_shstk )
>>> Should this be:
>>>     if ( IS_ENABLED(CONFIG_XEN_SHSTK) && cpu_has_xen_shstk )
>>>
>>> to match arch_livepatch_revive?
>> While it may look a little asymmetric, I think it's preferable
>> to is IS_ENABLED() only where really needed, i.e. here it
>> guarding code that otherwise may not build.
> 
> The reason for the asymmetry is because of the asm() block, which needs
> compiling out when we detect that we don't have a compatible assembler.
> 
> I was wondering whether I should make cpu_has_xen_shstk be false for
> !CONFIG_XEN_SHSTK, but that would be 4.15 work at this point.

Ah yes, this might then help with other code as well.

Jan
Ross Lagerwall June 26, 2020, 3:07 p.m. UTC | #7
On 2020-06-26 15:46, Andrew Cooper wrote:
> On 26/06/2020 15:26, Jan Beulich wrote:
>> On 26.06.2020 15:59, Ross Lagerwall wrote:
>>> On 2020-06-26 13:24, Andrew Cooper wrote:
>>>> @@ -56,18 +57,48 @@ int arch_livepatch_safety_check(void)
>>>>      return -EBUSY;
>>>>  }
>>>>  
>>>> -int arch_livepatch_quiesce(void)
>>>> +int noinline arch_livepatch_quiesce(void)
>>>>  {
>>>> +    /* If Shadow Stacks are in use, disable CR4.CET so we can modify CR0.WP. */
>>>> +    if ( cpu_has_xen_shstk )
>>> Should this be:
>>>     if ( IS_ENABLED(CONFIG_XEN_SHSTK) && cpu_has_xen_shstk )
>>>
>>> to match arch_livepatch_revive?
>> While it may look a little asymmetric, I think it's preferable
>> to is IS_ENABLED() only where really needed, i.e. here it
>> guarding code that otherwise may not build.
> 
> The reason for the asymmetry is because of the asm() block, which needs
> compiling out when we detect that we don't have a compatible assembler.
> 

In that case,

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Thanks
Andrew Cooper June 26, 2020, 3:08 p.m. UTC | #8
On 26/06/2020 16:07, Ross Lagerwall wrote:
> On 2020-06-26 15:46, Andrew Cooper wrote:
>> On 26/06/2020 15:26, Jan Beulich wrote:
>>> On 26.06.2020 15:59, Ross Lagerwall wrote:
>>>> On 2020-06-26 13:24, Andrew Cooper wrote:
>>>>> @@ -56,18 +57,48 @@ int arch_livepatch_safety_check(void)
>>>>>      return -EBUSY;
>>>>>  }
>>>>>  
>>>>> -int arch_livepatch_quiesce(void)
>>>>> +int noinline arch_livepatch_quiesce(void)
>>>>>  {
>>>>> +    /* If Shadow Stacks are in use, disable CR4.CET so we can modify CR0.WP. */
>>>>> +    if ( cpu_has_xen_shstk )
>>>> Should this be:
>>>>     if ( IS_ENABLED(CONFIG_XEN_SHSTK) && cpu_has_xen_shstk )
>>>>
>>>> to match arch_livepatch_revive?
>>> While it may look a little asymmetric, I think it's preferable
>>> to is IS_ENABLED() only where really needed, i.e. here it
>>> guarding code that otherwise may not build.
>> The reason for the asymmetry is because of the asm() block, which needs
>> compiling out when we detect that we don't have a compatible assembler.
>>
> In that case,
>
> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Thanks.  I've decided to clean this up in the (growing) series of 4.15
changes.

~Andrew

Patch
diff mbox series

diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 901fad96bf..49f0d902e5 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -12,6 +12,7 @@ 
 #include <xen/livepatch.h>
 #include <xen/sched.h>
 #include <xen/vm_event.h>
+#include <xen/virtual_region.h>
 
 #include <asm/fixmap.h>
 #include <asm/nmi.h>
@@ -56,18 +57,48 @@  int arch_livepatch_safety_check(void)
     return -EBUSY;
 }
 
-int arch_livepatch_quiesce(void)
+int noinline arch_livepatch_quiesce(void)
 {
+    /* If Shadow Stacks are in use, disable CR4.CET so we can modify CR0.WP. */
+    if ( cpu_has_xen_shstk )
+        write_cr4(read_cr4() & ~X86_CR4_CET);
+
     /* Disable WP to allow changes to read-only pages. */
     write_cr0(read_cr0() & ~X86_CR0_WP);
 
     return 0;
 }
 
-void arch_livepatch_revive(void)
+void noinline arch_livepatch_revive(void)
 {
     /* Reinstate WP. */
     write_cr0(read_cr0() | X86_CR0_WP);
+
+    /* Clobber dirty bits and reinstate CET, if applicable. */
+    if ( IS_ENABLED(CONFIG_XEN_SHSTK) && cpu_has_xen_shstk )
+    {
+        unsigned long tmp;
+
+        reset_virtual_region_perms();
+
+        write_cr4(read_cr4() | X86_CR4_CET);
+
+        /*
+         * Fix up the return address on the shadow stack, which currently
+         * points at arch_livepatch_quiesce()'s caller.
+         *
+         * Note: this is somewhat fragile, and depends on both
+         * arch_livepatch_{quiesce,revive}() being called from the same
+         * function, which is currently the case.
+         *
+         * Any error will result in Xen dying with #CP, and its too late to
+         * recover in any way.
+         */
+        asm volatile ("rdsspq %[ssp];"
+                      "wrssq %[addr], (%[ssp]);"
+                      : [ssp] "=&r" (tmp)
+                      : [addr] "r" (__builtin_return_address(0)));
+    }
 }
 
 int arch_livepatch_verify_func(const struct livepatch_func *func)
diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
index aa23918bce..4fbc02e35a 100644
--- a/xen/common/virtual_region.c
+++ b/xen/common/virtual_region.c
@@ -4,6 +4,7 @@ 
 
 #include <xen/init.h>
 #include <xen/kernel.h>
+#include <xen/mm.h>
 #include <xen/rcupdate.h>
 #include <xen/spinlock.h>
 #include <xen/virtual_region.h>
@@ -91,6 +92,20 @@  void unregister_virtual_region(struct virtual_region *r)
     remove_virtual_region(r);
 }
 
+#if defined(CONFIG_LIVEPATCH) && defined(CONFIG_XEN_SHSTK)
+void reset_virtual_region_perms(void)
+{
+    const struct virtual_region *region;
+
+    rcu_read_lock(&rcu_virtual_region_lock);
+    list_for_each_entry_rcu( region, &virtual_region_list, list )
+        modify_xen_mappings((unsigned long)region->start,
+                            ROUNDUP((unsigned long)region->end, PAGE_SIZE),
+                            PAGE_HYPERVISOR_RX);
+    rcu_read_unlock(&rcu_virtual_region_lock);
+}
+#endif
+
 void __init unregister_init_virtual_region(void)
 {
     BUG_ON(system_state != SYS_STATE_active);
diff --git a/xen/include/xen/virtual_region.h b/xen/include/xen/virtual_region.h
index e5e58ed96b..ba408eb87a 100644
--- a/xen/include/xen/virtual_region.h
+++ b/xen/include/xen/virtual_region.h
@@ -33,6 +33,7 @@  void setup_virtual_regions(const struct exception_table_entry *start,
 void unregister_init_virtual_region(void);
 void register_virtual_region(struct virtual_region *r);
 void unregister_virtual_region(struct virtual_region *r);
+void reset_virtual_region_perms(void);
 
 #endif /* __XEN_VIRTUAL_REGION_H__ */