Re: [PATCH 01/13] x86/entry: Fix AC assertion

[Andrew Cooper]

On 02/09/2020 16:58, Brian Gerst wrote:
> On Wed, Sep 2, 2020 at 9:38 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>> From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>>
>> The WARN added in commit 3c73b81a9164 ("x86/entry, selftests: Further
>> improve user entry sanity checks") unconditionally triggers on my IVB
>> machine because it does not support SMAP.
>>
>> For !SMAP hardware we patch out CLAC/STAC instructions and thus if
>> userspace sets AC, we'll still have it set after entry.
>>
>> Fixes: 3c73b81a9164 ("x86/entry, selftests: Further improve user entry sanity checks")
>> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
>> Acked-by: Andy Lutomirski <luto@xxxxxxxxxx>
>> ---
>>  arch/x86/include/asm/entry-common.h |   11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> --- a/arch/x86/include/asm/entry-common.h
>> +++ b/arch/x86/include/asm/entry-common.h
>> @@ -18,8 +18,16 @@ static __always_inline void arch_check_u
>>                  * state, not the interrupt state as imagined by Xen.
>>                  */
>>                 unsigned long flags = native_save_fl();
>> -               WARN_ON_ONCE(flags & (X86_EFLAGS_AC | X86_EFLAGS_DF |
>> -                                     X86_EFLAGS_NT));
>> +               unsigned long mask = X86_EFLAGS_DF | X86_EFLAGS_NT;
>> +
>> +               /*
>> +                * For !SMAP hardware we patch out CLAC on entry.
>> +                */
>> +               if (boot_cpu_has(X86_FEATURE_SMAP) ||
>> +                   (IS_ENABLED(CONFIG_64_BIT) && boot_cpu_has(X86_FEATURE_XENPV)))
>> +                       mask |= X86_EFLAGS_AC;
> Is the explicit Xen check necessary?  IIRC the Xen hypervisor will
> filter out the SMAP bit in the cpuid pvop.

The Xen check isn't anything to do with SMAP.

64bit PV guest kernels run in Ring3, so userspace's choice of AC for
real alignment check purposes needs to not leak into kernel context.

Xen's ABI for a user => kernel context switch should clear AC on behalf
of the kernel, but the fact still remains that if AC actually leaks into
context for whatever reason, stuff is going to break.

~Andrew