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

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

On 22/11/2019 18:08, George Dunlap wrote:
> On Fri, Nov 22, 2019 at 5:55 PM Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Juergen Gross <jgross@suse.com>
>> ---
>>  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@citrix.com>

That is the issue we're working around by not letting the L01 walk
encounter an NX superpage to begin with.

~Andrew

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