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

[Tim Deegan <tim@xen.org>]

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 have no problem with shadow-code functions taking pointers to
functions that take pointers to emulator context; and indeed I'd be
happy to put e.g. x86_insn_is_mem_write directly into the struct
x86_emulate_ops or pass it as another argument to x86_emulate().

Cheers,

Tim.

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