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

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

On 04/01/17 10:10, Tim Deegan wrote:
> At 02:22 -0700 on 04 Jan (1483496577), Jan Beulich wrote:
>>>>> On 03.01.17 at 18:29, <andrew.cooper3@citrix.com> wrote:
>>> On 03/01/17 16:19, Jan Beulich wrote:
>>>>>>> On 03.01.17 at 16:22, <andrew.cooper3@citrix.com> 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.
>> Let me get a 3rd opinion then - Tim, if such filtering was added for
>> shadow mode code, would you rather see them go straight to an
>> x86_insn_is_*() function, or have a proper sh_*() layer in between?
> I think checks on _kinds_ of instructions, like is_portio,
> is_mem_access &c are best provided as generic x86_insn_is_*.  I don't
> think I'd want to add sh_ wrappers that just called the x86_insn ones.
>
> I'd also be OK with an enum passed to the emulator and no callback
> function at all, if we can convince ourselves that every caller will
> want to check for exactly 0 or 1 classes, and no other filtering --
> maybe we can?

I considered suggesting this, but priv_op_validate() used by the PV path
really does need a full functions worth of flexibility.

A hybrid approach might also be an option if we end up with a lot of
simple tests like this, but I am not sure it is worth introducing at
this point.

~Andrew

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