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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/01/17 16:19, Jan Beulich wrote:
>>>> 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.

Given that one structure is embedded in the other, I am less concerned
about this being a layering violation.

I was specifically thinking along the line of not needing hvm and sh
stubs to call into x86_insn_is_mem_access(), as the hvm/sh nature isn't
relevant to the operation.

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

Well, not really.  When we choose to offer that facility, it should come
with an opt-in, and we can extend the validate function to permit
instructions in classes permitted by the domain featureset, but missing
in the host featureset.

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

Ok.

~Andrew

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