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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.01.17 at 16:22, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/01/17 13:10, Jan Beulich wrote:
>> --- 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.

I had thought about this, but it feels like a layering violation to
pass a pointer to a function taking x86_emulate_ctxt to functions
in the HVM emulation group. Even if it involves adding slightly more
code, I think it would better stay this way.

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

I can certainly do that (albeit I'd pass NULL for the FEP case
instead of a function permitting everything), yet that will
lock us further into the corner where actively emulating insns
without hardware support is rather difficult to achieve.

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

It doesn't belong anywhere - I found the comment wrong while
collecting the memory store insns, and putting this in a separate
patch didn't seem worthwhile. I've added a word to the commit
message.

Jan


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