Re: [PATCH for-4.14] x86/livepatch: Make livepatching compatible with CET Shadow Stacks

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/06/2020 14:41, Jan Beulich wrote:
> On 08.06.2020 22:02, Andrew Cooper wrote:
>> Do we ever write into .rodata?  AFAICT, we introduce new fuctions for
>> references to new .rodata, rather than modifying existing .rodata.  If 
>> however
>> we do modify .rodata, then the virtual regions need extending with 
>> information
>> about .rodata so we can zap those dirty bits as well.
> Inspired by looking at setup_virtual_regions(), do exception fixup
> or bug frame tables possibly get patched?

If they're not in .rodata, they really ought to be.

That said, neither of those tables should get touched - we add new
subset tables in the livepatch, which covers anything arising from
modified code.  This means we don't merge/resort the table on load, or
edit the table at all on unload.

>
>> @@ -58,6 +59,10 @@ int arch_livepatch_safety_check(void)
>>  
>>  int 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);
>>  
>> @@ -68,6 +73,29 @@ void 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.
>> +         */
>> +        asm volatile ("rdsspq %[ssp];"
>> +                      "wrssq %[addr], (%[ssp]);"
>> +                      : [ssp] "=&r" (tmp)
>> +                      : [addr] "r" (__builtin_return_address(0)));
>> +    }
> To be safe against LTO I think this wants accompanying with making
> both functions noinline.

Hmm true.

> As to the fragility - how about arch_livepatch_quiesce() returning
> __builtin_return_address(0) to its caller, for passing into here
> for verification? This would also make more noticeable that the
> two should be be called from the same function (or else the "token"
> would need passing further around).

This I'm less certain about, as its fairly invasive to common code, just
to bodge around something I don't expect to last very long into the 4.15
timeframe.

>
>> @@ -91,6 +92,18 @@ void unregister_virtual_region(struct virtual_region *r)
>>      remove_virtual_region(r);
>>  }
>>  
>> +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);
>> +}
> Doesn't this result in shattering the trailing (and currently still
> only) 2Mb page mapping .text in the xen.efi case?

Not any more or less than its already clobbered by this logic in the
alternatives path, I think?

>  With the
> expectation of the approach changing in 4.15 this may not need
> addressing, but should imo be mentioned as a shortcoming in the
> description then.
>
> Also - how about "restore" instead of "reset"?

Why?  We're not passing some state sideways into the new mappings -
we're resetting them to their expected values.

>
> Finally, while indeed not a lot of code, should it nevertheless go
> inside #ifdef CONFIG_LIVEPATCH?

Tbh, it could be CONFIG_XEN_SHSTK if we decide to go down that route.

~Andrew