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(-)
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 )
+ 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__ */
--
2.11.0
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
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
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
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
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
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
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
> -----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
© 2016 - 2024 Red Hat, Inc.