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

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

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)

> Subsequently we will want them to have different
> behavior, so as first step identify which one is which. For now, both
> groups of constructs alias one another.
> 
> Double underscore prefixes are retained only on __{get,put}_guest(), to
> allow still distinguishing them from their "checking" counterparts once
> they also get renamed (to {get,put}_guest()).
> 
> Since for them it's almost a full re-write, move what becomes
> {get,put}_unsafe_size() into the "common" uaccess.h (x86_64/*.h should
> disappear at some point anyway).
> 
> In __copy_to_user() one of the two casts in each put_guest_size()
> invocation gets dropped. They're not needed and did break symmetry with
> __copy_from_user().
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -776,9 +776,9 @@ shadow_write_entries(void *d, void *s, i
>      /* Because we mirror access rights at all levels in the shadow, an
>       * l2 (or higher) entry with the RW bit cleared will leave us with
>       * no write access through the linear map.
> -     * We detect that by writing to the shadow with __put_user() and
> +     * We detect that by writing to the shadow with put_unsafe() and
>       * using map_domain_page() to get a writeable mapping if we need to. */
> -    if ( __put_user(*dst, dst) )
> +    if ( put_unsafe(*dst, dst) )
>      {
>          perfc_incr(shadow_linear_map_failed);
>          map = map_domain_page(mfn);
> --- a/xen/arch/x86/pv/emul-gate-op.c
> +++ b/xen/arch/x86/pv/emul-gate-op.c
> @@ -40,7 +40,7 @@ static int read_gate_descriptor(unsigned
>           ((gate_sel >> 3) + !is_pv_32bit_vcpu(v) >=
>            (gate_sel & 4 ? v->arch.pv.ldt_ents
>                          : v->arch.pv.gdt_ents)) ||
> -         __get_user(desc, pdesc) )
> +         get_unsafe(desc, pdesc) )
>          return 0;
>  
>      *sel = (desc.a >> 16) & 0x0000fffc;
> @@ -59,7 +59,7 @@ static int read_gate_descriptor(unsigned
>      {
>          if ( (*ar & 0x1f00) != 0x0c00 ||
>               /* Limit check done above already. */
> -             __get_user(desc, pdesc + 1) ||
> +             get_unsafe(desc, pdesc + 1) ||
>               (desc.b & 0x1f00) )
>              return 0;
>  
> @@ -294,7 +294,7 @@ void pv_emulate_gate_op(struct cpu_user_
>          { \
>              --stkp; \
>              esp -= 4; \
> -            rc = __put_user(item, stkp); \
> +            rc = __put_guest(item, stkp); \
>              if ( rc ) \
>              { \
>                  pv_inject_page_fault(PFEC_write_access, \
> @@ -362,7 +362,7 @@ void pv_emulate_gate_op(struct cpu_user_
>                      unsigned int parm;
>  
>                      --ustkp;
> -                    rc = __get_user(parm, ustkp);
> +                    rc = __get_guest(parm, ustkp);
>                      if ( rc )
>                      {
>                          pv_inject_page_fault(0, (unsigned long)(ustkp + 1) - 
> rc);
> --- a/xen/arch/x86/pv/emulate.c
> +++ b/xen/arch/x86/pv/emulate.c
> @@ -34,13 +34,13 @@ int pv_emul_read_descriptor(unsigned int
>      if ( sel < 4 ||
>           /*
>            * Don't apply the GDT limit here, as the selector may be a Xen
> -          * provided one. __get_user() will fail (without taking further
> +          * provided one. get_unsafe() will fail (without taking further
>            * action) for ones falling in the gap between guest populated
>            * and Xen ones.
>            */
>           ((sel & 4) && (sel >> 3) >= v->arch.pv.ldt_ents) )
>          desc.b = desc.a = 0;
> -    else if ( __get_user(desc, gdt_ldt_desc_ptr(sel)) )
> +    else if ( get_unsafe(desc, gdt_ldt_desc_ptr(sel)) )
>          return 0;
>      if ( !insn_fetch )
>          desc.b &= ~_SEGMENT_L;
> --- a/xen/arch/x86/pv/iret.c
> +++ b/xen/arch/x86/pv/iret.c
> @@ -114,15 +114,15 @@ unsigned int compat_iret(void)
>      regs->rsp = (u32)regs->rsp;
>  
>      /* Restore EAX (clobbered by hypercall). */
> -    if ( unlikely(__get_user(regs->eax, (u32 *)regs->rsp)) )
> +    if ( unlikely(__get_guest(regs->eax, (u32 *)regs->rsp)) )
>      {
>          domain_crash(v->domain);
>          return 0;
>      }
>  
>      /* Restore CS and EIP. */
> -    if ( unlikely(__get_user(regs->eip, (u32 *)regs->rsp + 1)) ||
> -        unlikely(__get_user(regs->cs, (u32 *)regs->rsp + 2)) )
> +    if ( unlikely(__get_guest(regs->eip, (u32 *)regs->rsp + 1)) ||
> +        unlikely(__get_guest(regs->cs, (u32 *)regs->rsp + 2)) )
>      {
>          domain_crash(v->domain);
>          return 0;
> @@ -132,7 +132,7 @@ unsigned int compat_iret(void)
>       * Fix up and restore EFLAGS. We fix up in a local staging area
>       * to avoid firing the BUG_ON(IOPL) check in arch_get_info_guest.
>       */
> -    if ( unlikely(__get_user(eflags, (u32 *)regs->rsp + 3)) )
> +    if ( unlikely(__get_guest(eflags, (u32 *)regs->rsp + 3)) )
>      {
>          domain_crash(v->domain);
>          return 0;
> @@ -164,16 +164,16 @@ unsigned int compat_iret(void)
>          {
>              for (i = 1; i < 10; ++i)
>              {
> -                rc |= __get_user(x, (u32 *)regs->rsp + i);
> -                rc |= __put_user(x, (u32 *)(unsigned long)ksp + i);
> +                rc |= __get_guest(x, (u32 *)regs->rsp + i);
> +                rc |= __put_guest(x, (u32 *)(unsigned long)ksp + i);
>              }
>          }
>          else if ( ksp > regs->esp )
>          {
>              for ( i = 9; i > 0; --i )
>              {
> -                rc |= __get_user(x, (u32 *)regs->rsp + i);
> -                rc |= __put_user(x, (u32 *)(unsigned long)ksp + i);
> +                rc |= __get_guest(x, (u32 *)regs->rsp + i);
> +                rc |= __put_guest(x, (u32 *)(unsigned long)ksp + i);
>              }
>          }
>          if ( rc )
> @@ -189,7 +189,7 @@ unsigned int compat_iret(void)
>              eflags &= ~X86_EFLAGS_IF;
>          regs->eflags &= ~(X86_EFLAGS_VM|X86_EFLAGS_RF|
>                            X86_EFLAGS_NT|X86_EFLAGS_TF);
> -        if ( unlikely(__put_user(0, (u32 *)regs->rsp)) )
> +        if ( unlikely(__put_guest(0, (u32 *)regs->rsp)) )
>          {
>              domain_crash(v->domain);
>              return 0;
> @@ -205,8 +205,8 @@ unsigned int compat_iret(void)
>      else if ( ring_1(regs) )
>          regs->esp += 16;
>      /* Return to ring 2/3: restore ESP and SS. */
> -    else if ( __get_user(regs->ss, (u32 *)regs->rsp + 5) ||
> -              __get_user(regs->esp, (u32 *)regs->rsp + 4) )
> +    else if ( __get_guest(regs->ss, (u32 *)regs->rsp + 5) ||
> +              __get_guest(regs->esp, (u32 *)regs->rsp + 4) )
>      {
>          domain_crash(v->domain);
>          return 0;
> --- 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?

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?

Thanks, Roger.