Re: [PATCH] x86/HVM: restrict permitted instructions during special purpose emulation

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 03/01/17 13:10, Jan Beulich wrote:
> Most invocations of the instruction emulator are for VM exits where the
> set of legitimate instructions (i.e. ones capable of causing the
> respective exit) is rather small. Restrict the permitted sets via a new
> callback, at once eliminating the abuse of handle_mmio() for non-MMIO
> operations.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: Better way to cover FPU/SIMD insns in x86_insn_is_mem_write()?

Not that I can see.

>
> Note that hvm_emulate_is_mem_*() (for now) intentionally don't include
> implicit memory operands: I don't think we mean to support namely
> the stack to live in MMIO, but otoh we may need to permit that.
>
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1039,6 +1039,17 @@ static int hvmemul_cmpxchg(
>      return hvmemul_write(seg, offset, p_new, bytes, ctxt);
>  }
>  
> +static int hvmemul_validate(
> +    const struct x86_emulate_state *state,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    struct hvm_emulate_ctxt *hvmemul_ctxt =
> +        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> +
> +    return hvmemul_ctxt->validate ? hvmemul_ctxt->validate(state, hvmemul_ctxt)
> +                                  : X86EMUL_OKAY;

There is nothing hvm-specific about any of the validation functions, and
x86_insn_is_{portio,cr_access,is_invlpg} seem more generally useful than
hvm-specific varients.

Do you forsee any validation which would need to peek into hvmeml_ctxt? 
I can't think of anything off the top of my head.

If not, this would be cleaner and shorter to have an x86emul_validate_t
based interface, always passing const struct x86_emulate_ctxt *ctxt.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4004,7 +4004,7 @@ void hvm_ud_intercept(struct cpu_user_re
>          cur->domain->arch.x86_vendor != boot_cpu_data.x86_vendor;
>      struct hvm_emulate_ctxt ctxt;
>  
> -    hvm_emulate_init_once(&ctxt, regs);
> +    hvm_emulate_init_once(&ctxt, NULL, regs);

Please could we have a validation function here which, for the
opt_hvm_fep case permits everything, and for the cross-vendor case
permits only SYS{CALL,RET,ENTER,EXIT}?

This severely limits the attack surface even for a VM configured in
cross-vendor mode, and we only need to cope with instructions which have
different #UD behaviour between vendors.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -3774,7 +3774,7 @@ x86_emulate(
>                  emulate_fpu_insn_memsrc("flds", src.val);
>                  dst.type = OP_NONE;
>                  break;
> -            case 2: /* fstp m32fp */
> +            case 2: /* fst m32fp */

This change looks like it is spurious from a different patch?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel