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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

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

> @@ -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? 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"?

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

Jan