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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
