Re: [Xen-devel] [PATCH RFC] x86/hvm: unify HVM and PVH hypercall tables.

[George Dunlap <dunlapg@xxxxxxxxx>]

On Thu, May 8, 2014 at 4:31 PM, Tim Deegan <tim@xxxxxxx> wrote:
> Stage one of many in merging PVH and HVM code in the hypervisor.
>
> This exposes a few new hypercalls to HVM guests, all of which were
> already available to PVH ones:
>
>  - XENMEM_memory_map / XENMEM_machine_memory_map / XENMEM_machphys_mapping:
>    These are basically harmless, if a bit useless to plain HVM.
>
>  - VCPUOP_send_nmi / VCPUOP_initialise / VCPUOP[_is]_up / VCPUOP_down
>    This will eventually let HVM guests bring up APs the way PVH ones do.
>    For now, the VCPUOP_initialise paths are still gated on is_pvh.
>  - VCPUOP_get_physid
>    Harmless.
>
>  - __HYPERVISOR_platform_op (XSM_PRIV callers only).
>
>  - __HYPERVISOR_mmuext_op.
>    The pagetable manipulation MMUEXT ops are already denied to
>    paging_mode_refcounts() domains; the baseptr ones are already
>    denied to paging_mode_translate() domains.
>    I have restricted MMUEXT[UN]MARK_SUPER to !paging_mode_refcounts()
>    domains as well, as I can see no need for them in PVH.
>    That leaves TLB and cache flush operations and MMUEXT_CLEAR_PAGE /
>    MMUEXT_COPY_PAGE, all of which are OK.
>
>  - PHYSDEVOP_* (only for hardware control domains).
>    Also make ops that touch PV vcpu state (PHYSDEVOP_set_iopl and
>    PHYSDEVOP_set_iobitmap) gate on is_pv rather than !is_pvh.
>
> PVH guests can also see a few hypercalls that they couldn't before,
> but which were already available to HVM guests:
>
>  - __HYPERVISOR_set_timer_op
>
>  - __HYPERVISOR_tmem_op
>
> Signed-off-by: Tim Deegan <tim@xxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>

In principle looks good.  I've gone through the code and it all looks
correct.  I haven't done an audit of the various hypercalls yet.

One thing to consider is that regardless of whether a hypercall is
safe for HVM guests *if implemented correctly*, every additional
hypercall exposed increases the risk that an attacker will be able to
find one which is *not* implemented correctly and be able to take
advantage of it.

Obviously the *best* solution to that would be Flask, but AFAICT it's
not very widely used.

On the whole, simplifying the code by unification is still probably
better though.

 -George

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