Re: [PATCH 02/17] x86: split __{get,put}_user() into "guest" and "unsafe" variants

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.02.2021 15:55, Roger Pau Monné wrote:
> On Thu, Jan 14, 2021 at 04:04:11PM +0100, Jan Beulich wrote:
>> The "guest" variants are intended to work with (potentially) fully guest
>> controlled addresses, while the "unsafe" variants are not. (For
>> descriptor table accesses the low bits of the addresses may still be
>> guest controlled, but this still won't allow speculation to "escape"
>> into unwanted areas.)
> 
> Descriptor table is also in guest address space, and hence would be
> fine using the _guest accessors? (even if not in guest control and
> thus unsuitable as an speculation vector)

No, we don't access descriptor tables in guest space, I don't
think. See read_gate_descriptor() for an example. After all PV
guests don't even have the full concept of self-managed (in
their VA space) descriptor tables (GDT gets specified in terms
of frames, while LDT gets specified in terms of (VA,size)
tuples, but just for Xen to read the underlying page table
entries upon 1st access).

>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -274,7 +274,7 @@ static void compat_show_guest_stack(stru
>>      {
>>          if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask )
>>              break;
>> -        if ( __get_user(addr, stack) )
>> +        if ( get_unsafe(addr, stack) )
>>          {
>>              if ( i != 0 )
>>                  printk("\n    ");
>> @@ -343,7 +343,7 @@ static void show_guest_stack(struct vcpu
>>      {
>>          if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask )
>>              break;
>> -        if ( __get_user(addr, stack) )
>> +        if ( get_unsafe(addr, stack) )
> 
> Shouldn't accessing the guest stack use the _guest accessors?

Hmm, yes indeed.

> Or has this address been verified by Xen and not in idrect control of
> the guest, and thus can't be used for speculation purposes?
> 
> I feel like this should be using the _guest accessors anyway, as the
> guest stack is an address in guest space?

I think this being a debugging function only, not directly
accessible by guests, is what made me think speculation is
not an issue here and hence the "unsafe" variants are fine
to use (they're slightly cheaper after all, once the
subsequent changes are in place). But I guess I will better
switch these two around.

Jan