Re: [PATCH 2/2] x86/shskt: Disable CET-SS on parts susceptible to fractured updates

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 04/01/2023 3:44 pm, Jan Beulich wrote:
> On 04.01.2023 12:11, Andrew Cooper wrote:
>> Refer to Intel SDM Rev 70 (Dec 2022), Vol3 17.2.3 "Supervisor Shadow Stack
>> Token".
>>
>> Architecturally, an event delivery which starts in CPL<3 and switches shadow
>> stack will first validate the Supervisor Shadow Stack Token (setting the busy
>> bit), then pushes CS/LIP/SSP.  One example of this is an NMI interrupting 
>> Xen.
>>
>> Some CPUs suffer from an issue called fracturing, whereby a fault/vmexit/etc
>> between setting the busy bit and completing the event injection renders the
>> action non-restartable, because when it comes time to restart, the busy bit 
>> is
>> found to be already set.
>>
>> This is far more easily encountered under virt, yet it is not the fault of 
>> the
>> hypervisor, nor the fault of the guest kernel.  The fault lies somewhere
>> between the architectural specification, and the uarch behaviour.
>>
>> Intel have allocated CPUID.7[1].ecx[18] CET_SSS to enumerate that supervisor
>> shadow stacks are safe to use.  Because of how Xen lays out its shadow 
>> stacks,
>> fracturing is not expected to be a problem on native.
> IOW that's the "contained in an aligned 32-byte region" constraint which we
> meet, aiui.

For practical purposes, it is "contained within a single cache line".

AMD's position is "if the OS doesn't have suitable alignment, or doesn't
place the shstk on WB memory, it gets to keep any resulting pieces". 
They have confirmed that if there is suitable alignment and it is on a
WB mapping, then no vmexit can occur which would fracture the the shadow
stack.

Intel retroactively tightened the constraints in microcode on TGL/ADL so
that MSR_PL?_SSP (or ISST pointers from memory) would suffer #GP if they
didn't refer to the top word in an aligned 32-byte region.

But this provide to be insufficient to fix the problem, hence the new
CPUID bit, and recommendation to disable supervisor shadow stacks by
default under virt.

>> Detect this case on boot and default to not using shstk if virtualised.
>> Specifying `cet=shstk` on the command line will override this heuristic and
>> enable shadow stacks irrespective.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with one nit (below).
>
>> This ideally wants backporting to Xen 4.14.  I have no idea how likely it is
>> to need to backport the prerequisite patch for new feature words, but we've
>> already had to do that once for security patches.  OTOH, I have no idea how
>> easy it is to trigger in non-synthetic cases.
> Plus: How likely is it that Xen actually is used virtualized in production?

All of our gitlab smoke tests, a larger part of QubesOS's CI, and
non-default PV-shim configurations.

As soon as we get guest CET working, then part of OSStest, and a portion
of XenServer's general testing too.

It does need backporting to all general support trees.  (When I first
started working on this problem with Intel, that would have included
4.14 too...)

>> @@ -1099,11 +1095,45 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>      early_cpu_init();
>>  
>>      /* Choose shadow stack early, to set infrastructure up appropriately. */
>> -    if ( opt_xen_shstk && boot_cpu_has(X86_FEATURE_CET_SS) )
>> +    if ( !boot_cpu_has(X86_FEATURE_CET_SS) )
>> +        opt_xen_shstk = 0;
>> +
>> +    if ( opt_xen_shstk )
>>      {
>> -        printk("Enabling Supervisor Shadow Stacks\n");
>> +        /*
>> +         * Some CPUs suffer from Shadow Stack Fracturing, an issue whereby a
>> +         * fault/VMExit/etc between setting a Supervisor Busy bit and the
>> +         * event delivery completing renders the operation non-restartable.
>> +         * On restart, event delivery will find the Busy bit already set.
>> +         *
>> +         * This is a problem on bare metal, but outside of synthetic cases 
>> or
>> +         * a very badly timed #MC, it's not believed to problem.  It is a 
>> much
> Nit: "... to be a problem."

Fixed, thanks.

~Andrew