Re: [Xen-devel] [PATCH] x86/ctxt-switch: Document and improve GDT handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/07/2019 11:00, Jan Beulich wrote:
> On 04.07.2019 19:57, Andrew Cooper wrote:
>> write_full_gdt_ptes() has a latent bug.  Using virt_to_mfn() and iterating
>> with (mfn + i) is wrong, because of PDX compression.  The context switch path
>> only functions correctly because NR_RESERVED_GDT_PAGES is 1.
> Whether this is a (latent) bug depends on how the allocation gets
> done. As long as it's a single alloc_xenheap_pages(), this is
> perfectly fine. There are no individual allocations which can span
> a PDX compression hole (or else MFN or struct page pointer
> arithmetic wouldn't work either, independent of the involvement of
> a virtual address).

Hmm - Its still very deceptive code.

>
>> Also, it should now be very obvious to people that Xen's current GDT handling
>> for non-PV vcpus is a recipe subtle bugs, if we ever manage to execute a 
>> stray
>> mov/pop %sreg instruction.  We really ought to have Xen's regular GDT in an
>> area where slots 0-13 are either mapped to the zero page, or not present, so
>> we don't risk loading a non-faulting garbage selector.
> Well, there's certainly room for improvement, but loading a stray
> selector seems pretty unlikely an event to happen, and the
> respective code having got slipped in without anyone noticing.
> Other than in context switching code I don't think there are many
> places at all where we write to the selector registers.

There are however many places where we write some bytes into a stub and
then execute them.

This isn't a security issue.  There aren't any legitimate codepaths for
which is this a problem, but there are plenty of cascade failures where
this is liable to make a bad situation worse is weird hard-to-debug ways.

Not to mention that for security hardening purposes, we should be using
a RO mapping to combat sgdt or fixed-ABI knowledge from an attacker.

And on that note... nothing really updates the full GDT via the
perdomain mappings, so I think that can already move to being RO.  This
does depend on the fact that noone has used segmented virtual memory
since long before Xen was a thing.  We can trap and emulate the setting
of A bits, and I bet that path will never get hit even with old PV guests.

>> @@ -1718,15 +1737,12 @@ static void __context_switch(void)
>>   
>>       psr_ctxt_switch_to(nd);
>>   
>> -    gdt = !is_pv_32bit_domain(nd) ? per_cpu(gdt_table, cpu) :
>> -                                    per_cpu(compat_gdt_table, cpu);
>> -
>>       if ( need_full_gdt(nd) )
>> -        write_full_gdt_ptes(gdt, n);
>> +        update_xen_slot_in_full_gdt(n, cpu);
>>   
>>       if ( need_full_gdt(pd) &&
>>            ((p->vcpu_id != n->vcpu_id) || !need_full_gdt(nd)) )
>> -        load_default_gdt(gdt, cpu);
>> +        load_default_gdt(cpu);
>  From looking at this transformation I cannot see how, as said in
> the description and as expressed by removing the gdt parameter
> from load_default_gdt(), the gdt having got passed in here would
> always have been per_cpu(gdt_table, cpu). It pretty clearly was
> the compat one for nd being 32-bit PV. What am I missing?

To be perfectly honest, I wrote "how it {does,should} logically work",
then adjusted the code.

> Or is the description perhaps instead meaning to say that it doesn't
> _need_ to be the compat one that we load here, as in case it is
> the subsequent load_full_gdt() will replace it again anyway?

lgdt is an expensive operation.  I hadn't even spotted that we are doing
it twice on that path.  There is surely some room for improvement here
as well.

I wonder if caching the last gdt base address per cpu would be a better
option, and only doing a "lazy" lgdt.  It would certainly simply the
"when should I lgdt?" logic.

>
>> @@ -2059,6 +2061,14 @@ void __init trap_init(void)
>>           }
>>       }
>>   
>> +    /* Cache {,compat_}gdt_table_l1e now that physically relocation is 
>> done. */
> "physical relocation" or "physically relocating"?

Oops.  I'll go with the former.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel