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 <jbeul...@suse.com>
>
> --- 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.
