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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.04.2023 19:54, Andrew Cooper wrote:
> On 18/04/2023 6:30 pm, Andrew Cooper wrote:
>> On 18/04/2023 12:10 pm, Andrew Cooper wrote:
>>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>>> index 36a07ef77eae..98529215ddec 100644
>>> @@ -5879,6 +5880,75 @@ int destroy_xen_mappings(unsigned long s, unsigned 
>>> long e)
>>>      return modify_xen_mappings(s, e, _PAGE_NONE);
>>>  }
>>>  
>>> +/*
>>> + * 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 present flags, and over present mappings.
>>> + * Must be called on leaf page boundaries, i.e. s and e must not be in the
>>> + * middle of a superpage.
>>> + */
>>> +void init_or_livepatch modify_xen_mappings_lite(
>>> +    unsigned long s, unsigned long e, unsigned int _nf)
>>> +{
>>> +    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)
>>> +    fm = put_pte_flags(FLAGS_MASK);
>>> +    nf = put_pte_flags(_nf & FLAGS_MASK);
>>> +#undef FLAGS_MASK
>>> +
>>> +    ASSERT(nf & _PAGE_PRESENT);
>>> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE) && s >= XEN_VIRT_START);
>>> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE) && e <= XEN_VIRT_END);
>>> +
>>> +    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);
>>> +            ASSERT(e - v >= (1UL << L2_PAGETABLE_SHIFT));
>> On second thoughts, no.  This has just triggered in my final sanity
>> testing before pushing.
>>
>> Currently debugging.
> 
> (XEN) livepatch: lp: Applying 1 functions
> (XEN) *** ML (ffff82d040200000, ffff82d0403b4000, 0x163)
> (XEN)   l2t[001] SP: 000000009f4001a1->000000009f4001e3  (v
> ffff82d040200000, e ffff82d0403b4000)
> (XEN) hi_func: Hi! (called 1 times)
> (XEN) Hook executing.
> (XEN) *** ML (ffff82d040200000, ffff82d0403b4000, 0x121)
> (XEN)   l2t[001] SP: 000000009f4001e3->000000009f4001a1  (v
> ffff82d040200000, e ffff82d0403b4000)
> (XEN) livepatch: module metadata:
> 
> When Xen is using forced 2M alignment, the virtual_region entry for
> .text isn't aligned up to the end of the region.
> 
> So the final bullet point is actually wrong.  I'm going to relax it to
> say that it is the callers responsibility to make sure that bad things
> don't happen if s or e are in the middle of a superpage, because I'm not
> changing how virtual_region works to satisfy this assert.

I.e. you'll drop both assertions, not just the one added recently? Or
else what you say you'll tweak the comment to isn't going to be
consistent with code. (I ask because you say "this", not "these", which
is a little ambiguous in its meaning here.)

Jan