Re: [Xen-devel] [PATCH for-4.13] x86/vvmx: Fix livelock with XSA-304 fix

[George Dunlap <dunlapg@xxxxxxxxx>]

On Fri, Nov 22, 2019 at 5:55 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> It turns out that the XSA-304 / CVE-2018-12207 fix of disabling executable
> superpages doesn't work well with the nested p2m code.
>
> Nested virt is experimental and not security supported, but is useful for
> development purposes.  In order to not regress the status quo, disable the
> XSA-304 workaround until the nested p2m code can be improved.
>
> Introduce a per-domain exec_sp control and set it based on the current
> opt_ept_exec_sp setting.  Take the oppotunity to omit a PVH hardware domain
> from the performance hit, because it is already permitted to DoS the system in
> such ways as issuing a reboot.
>
> When nested virt is enabled on a domain, force it to using executable
> superpages and rebuild the p2m.
>
> Having the setting per-domain involves rearranging the internals of
> parse_ept_param_runtime() but it still retains the same overall semantics -
> for each applicable domain whose setting needs to change, rebuild the p2m.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Juergen Gross <jgross@xxxxxxxx>
> ---
>  xen/arch/x86/hvm/vmx/vmcs.c        | 31 +++++++++++++++++++++++--------
>  xen/arch/x86/hvm/vmx/vmx.c         |  6 ++++++
>  xen/arch/x86/hvm/vmx/vvmx.c        | 13 +++++++++++++
>  xen/arch/x86/mm/p2m-ept.c          |  2 +-
>  xen/include/asm-x86/hvm/vmx/vmcs.h |  6 ++++++
>  5 files changed, 49 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 477c968409..f10f6b78ec 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -31,6 +31,7 @@
>  #include <asm/xstate.h>
>  #include <asm/hvm/hvm.h>
>  #include <asm/hvm/io.h>
> +#include <asm/hvm/nestedhvm.h>
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/vmx/vmx.h>
>  #include <asm/hvm/vmx/vvmx.h>
> @@ -97,6 +98,7 @@ custom_param("ept", parse_ept_param);
>
>  static int parse_ept_param_runtime(const char *s)
>  {
> +    struct domain *d;
>      int val;
>
>      if ( !cpu_has_vmx_ept || !hvm_funcs.hap_supported ||
> @@ -110,18 +112,31 @@ static int parse_ept_param_runtime(const char *s)
>      if ( (val = parse_boolean("exec-sp", s, NULL)) < 0 )
>          return -EINVAL;
>
> -    if ( val != opt_ept_exec_sp )
> +    opt_ept_exec_sp = val;
> +
> +    rcu_read_lock(&domlist_read_lock);
> +    for_each_domain ( d )
>      {
> -        struct domain *d;
> +        /* PV, or HVM Shadow domain?  Not applicable. */
> +        if ( !paging_mode_hap(d) )
> +            continue;
>
> -        opt_ept_exec_sp = val;
> +        /* Hardware domain? Not applicable. */
> +        if ( is_hardware_domain(d) )
> +            continue;
>
> -        rcu_read_lock(&domlist_read_lock);
> -        for_each_domain ( d )
> -            if ( paging_mode_hap(d) )
> -                p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_rw);
> -        rcu_read_unlock(&domlist_read_lock);
> +        /* Nested Virt?  Broken and exec_sp forced on to avoid livelocks. */
> +        if ( nestedhvm_enabled(d) )
> +            continue;
> +
> +        /* Setting already matches?  No need to rebuild the p2m. */
> +        if ( d->arch.hvm.vmx.exec_sp == val )
> +            continue;
> +
> +        d->arch.hvm.vmx.exec_sp = val;
> +        p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_rw);
>      }
> +    rcu_read_unlock(&domlist_read_lock);
>
>      printk("VMX: EPT executable superpages %sabled\n",
>             val ? "en" : "dis");
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 6a5eeb5c13..a71df71bc1 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -404,6 +404,12 @@ static int vmx_domain_initialise(struct domain *d)
>
>      d->arch.ctxt_switch = &csw;
>
> +    /*
> +     * Work around CVE-2018-12207?  The hardware domain is already permitted
> +     * to reboot the system, so doesn't need mitigating against DoS's.
> +     */
> +    d->arch.hvm.vmx.exec_sp = is_hardware_domain(d) || opt_ept_exec_sp;
> +
>      if ( !has_vlapic(d) )
>          return 0;
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 6696bd6240..5dd00e11b5 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -63,10 +63,23 @@ void nvmx_cpu_dead(unsigned int cpu)
>
>  int nvmx_vcpu_initialise(struct vcpu *v)
>  {
> +    struct domain *d = v->domain;
>      struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
>      struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
>      struct page_info *pg = alloc_domheap_page(NULL, 0);
>
> +    /*
> +     * Gross bodge.  The nested p2m logic can't cope with the CVE-2018-12207
> +     * workaround of using NX EPT superpages, and livelocks.  Nested HVM 
> isn't
> +     * security supported, so disable the workaround until the nested p2m
> +     * logic can be improved.
> +     */
> +    if ( !d->arch.hvm.vmx.exec_sp )
> +    {
> +        d->arch.hvm.vmx.exec_sp = true;
> +        p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_rw);

There wasn't an issue with nested guests having to deal with the
changed entry type?

Assuming the answer to that is "no":

Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel