Message ID | 20200626122408.19151-1-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2,for-4.14] x86/livepatch: Make livepatching compatible with CET Shadow Stacks | expand |
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
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
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__ */
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(-)