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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/04/2023 7:41 am, Jan Beulich wrote:
> 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?

Yes, I dropped both.  I didn't think it was reasonable to keep one but
not the other.

~Andrew