Re: [PATCH] x86/livepatch: Fix livepatch application when CET is active

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2023 21:58, Andrew Cooper wrote:
> Right now, trying to apply a livepatch on any system with CET shstk (AMD Zen3
> or later, Intel Tiger Lake or Sapphire Rapids and later) fails as follows:
> 
>   (XEN) livepatch: lp: Verifying enabled expectations for all functions
>   (XEN) common/livepatch.c:1591: livepatch: lp: timeout is 30000000ns
>   (XEN) common/livepatch.c:1703: livepatch: lp: CPU28 - IPIing the other 127 
> CPUs
>   (XEN) livepatch: lp: Applying 1 functions
>   (XEN) hi_func: Hi! (called 1 times)
>   (XEN) Hook executing.
>   (XEN) Assertion 'local_irq_is_enabled() || cpumask_subset(mask, 
> cpumask_of(cpu))' failed at arch/x86/smp.c:265
>   (XEN) *** DOUBLE FAULT ***
>   <many double faults>
> 
> The assertion failure is from a global (system wide) TLB flush initiated by
> modify_xen_mappings().  I'm not entirely sure when this broke, and I'm not
> sure exactly what causes the #DF's, but it doesn't really matter either
> because they highlight a latent bug that I'd overlooked with the CET-SS vs
> patching work the first place.

Which perhaps warrants a Fixes: tag at least for that latter change you
mention?

> While we're careful to arrange for the patching CPU to avoid encountering
> non-shstk memory with transient shstk perms, other CPUs can pick these
> mappings up too if they need to re-walk for uarch reasons.
> 
> Another bug is that for livepatching, we only disable CET if shadow stacks are
> in use.  Running on Intel CET systems when Xen is only using CET-IBT will
> crash in arch_livepatch_quiesce() when trying to clear CR0.WP with CR4.CET
> still active.
> 
> Also, we never went and cleared the dirty bits on .rodata.  This would
> matter (for the same reason it matters on .text - it becomes a valid target
> for WRSS), but we never actually patch .rodata anyway.

Maybe worth making explicit that this (the clearing of D bits for .rodata)
also isn't changed here? Otherwise this reads as if you meant to deal with
this as well.

> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -382,24 +382,28 @@ static int __init cf_check nmi_apply_alternatives(
>       */
>      if ( !(alt_done & alt_todo) )
>      {
> -        unsigned long cr0, cr4;
> -
> -        cr0 = read_cr0();
> -        cr4 = read_cr4();
> -
> -        if ( cr4 & X86_CR4_CET )
> -            write_cr4(cr4 & ~X86_CR4_CET);
> -
> -        /* Disable WP to allow patching read-only pages. */
> -        write_cr0(cr0 & ~X86_CR0_WP);
> +        /*
> +         * Relax perms on .text to be RWX, so we can modify them.
> +         *
> +         * This relaxes perms globally, but we run ahead of bringing APs
> +         * online, so only have our own TLB to worry about.
> +         */
> +        modify_xen_mappings_lite(XEN_VIRT_START + MB(2),
> +                                 (unsigned long)&__2M_text_end,
> +                                 PAGE_HYPERVISOR_RWX);
> +        flush_local(FLUSH_TLB_GLOBAL);
>  
>          _apply_alternatives(__alt_instructions, __alt_instructions_end,
>                              alt_done);
>  
> -        write_cr0(cr0);
> -
> -        if ( cr4 & X86_CR4_CET )
> -            write_cr4(cr4);
> +        /*
> +         * Reinstate perms on .text to be RW.  This also cleans out the dirty

I suppose you mean RX here, matching ...

> +         * bits, which matters when CET Shstk is active.
> +         */
> +        modify_xen_mappings_lite(XEN_VIRT_START + MB(2),
> +                                 (unsigned long)&__2M_text_end,
> +                                 PAGE_HYPERVISOR_RX);

... the code.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5879,6 +5879,77 @@ int destroy_xen_mappings(unsigned long s, unsigned 
> long e)
>      return modify_xen_mappings(s, e, _PAGE_NONE);
>  }
>  
> +#if defined(CONFIG_LIVEPATCH) && defined(CONFIG_HAS_ALTERNATIVE)

In line with your observation that this wants to be ||, ...

> +/*
> + * Similar to modify_xen_mappings(), but used by the alternatives and
> + * livepatch in weird contexts.  All synchronization, TLB flushing, etc is 
> the
> + * responsibility of the caller, and *MUST* not be introduced here.
> + *
> + * Must be limited to XEN_VIRT_{START,END}, i.e. over l2_xenmap[].
> + * Must be called with preset flags, and over present mappings.

(s/preset/present/ ?)

> + * Must be called on leaf page boundaries.
> + */
> +void modify_xen_mappings_lite(unsigned long s, unsigned long e, unsigned int 
> _nf)

... perhaps use init_or_livepatch here? At which point the #if may want
to go away, as in the !LIVEPATCH case the code then will be discarded
post-init anyway? The more that HAS_ALTERNATIVE is always true on x86
anyway.

> +{
> +    unsigned long v = s, fm, nf;
> +
> +    /* Set of valid PTE bits which may be altered. */
> +#define FLAGS_MASK 
> (_PAGE_NX|_PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_RW|_PAGE_PRESENT)
> +    _nf &= FLAGS_MASK;
> +
> +    fm = put_pte_flags(FLAGS_MASK);
> +    nf = put_pte_flags(_nf);
> +
> +    ASSERT(nf & _PAGE_PRESENT);
> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE) && s >= XEN_VIRT_START);
> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE) && e <= XEN_VIRT_END);

I can see why you want s page-aligned, but does e really need to be?

> +    while ( v < e )
> +    {
> +        l2_pgentry_t *pl2e = &l2_xenmap[l2_table_offset(v)];
> +        l2_pgentry_t l2e = l2e_read_atomic(pl2e);
> +        unsigned int l2f = l2e_get_flags(l2e);
> +
> +        ASSERT(l2f & _PAGE_PRESENT);
> +
> +        if ( l2e_get_flags(l2e) & _PAGE_PSE )
> +        {
> +            ASSERT(l1_table_offset(v) == 0);
> +
> +            l2e_write_atomic(pl2e, l2e_from_intpte((l2e.l2 & ~fm) | nf));
> +
> +            v += 1UL << L2_PAGETABLE_SHIFT;
> +            continue;
> +        }
> +
> +        /* else decend to l1 */

Nit: "descend"?

Jan