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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"Ross Lagerwall" <ross.lagerwall@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] x86/livepatch: Fix livepatch application when CET is active
Date: Mon, 17 Apr 2023 12:27:13 +0100	[thread overview]
Message-ID: <f1ffbf21-e6e2-b3cc-5c02-2382c052d20e@citrix.com> (raw)
In-Reply-To: <1cde4c30-bc48-7170-d465-11ed8617449f@suse.com>

On 17/04/2023 11:28 am, Jan Beulich wrote:
> 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?

Hmm yes.  I meant to do that and forgot.

>
>> 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.

Well, it is dealt with, but in a roundabout way.

With this patch in place, we don't relax the perms on .rodata, and never
crash in either alternatives or livepatching.

So we never actually write to .rodata, and never set D bits, so there's
nothing to clean up.

If in the future we do find a usecase that involves writing to .rodata,
then we will need to relax the perms too, and the D bits will be cleared
as a side effect of re-tightening.  This will also involves extending
virtual_region with more than just the .text reference.

>
>> --- 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.

Oops yes.

>
>> --- 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 ||, ...
>
>> + * 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?

Ah yes, missed that.

>  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.

I was considering if there was a nicer way to do this.  One idea was to
end up with it in some kind of lib-y form so it gets pulled in on
demand.  But that wouldn't cope nicely with putting it in .init for the
!LIVEPATCH case.

I think I'll just go with init_or_livepatch and drop the ifdefary.

>> +{
>> +    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?

To be honest, I copied this straight from modify_xen_mappings().

I think the logic will work without it being aligned, but I'd also
consider it an error to pass in a non-aligned end, seeing as this
function strictly operates on pagetable granularity.

~Andrew