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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2020 13:56, Andrew Cooper wrote:
> On 15/06/2020 16:16, Jan Beulich wrote:
>>>>> @@ -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.
>> I don't see it  being invasive - there's a new local variable needed
>> in both of apply_payload() and revert_payload(), and of course the
>> call sites would need adjustment.
> 
> Exactly - the call site adjustment is what makes it invasive in common
> code, and all other architectures.
> 
> Any getting this wrong will die with #CP[near ret] anyway.
> 
> The only thing passing that value around would do is let you tweak the
> error message slightly before we crash out.

Well, okay - I'm not a maintainer of that part of the code anyway.

Jan