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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 09, 2021 at 04:14:41PM +0100, Jan Beulich wrote:
> 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).

I see, read_gate_descriptor uses gdt_ldt_desc_ptr which points into
the per-domain Xen virt space, so it's indeed an address in Xen
address space. I realize it doesn't make sense for the descriptor
table to be in guest address space, as it's not fully under guest
control.

> >> --- 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.

With that:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.