Re: [Xen-devel] [PATCH V2 2/2] x86/hvm: fix domain crash when CR3 has the noflush bit set

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

"

On Tue, Jan 30, 2018 at 2:16 AM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> The emulation layers of Xen lack PCID support, and as we only offer
> PCID to HAP guests, all writes to CR3 are handled by hardware,
> except when introspection is involved. Consequently, trying to set
> CR3 when the noflush bit is set in hvm_set_cr3() leads to domain
> crashes. The workaround is to clear the noflush bit in
> hvm_set_cr3(). CR3 values in hvm_monitor_cr() are also sanitized.
> Additionally, a bool parameter now propagates to
> {svm,vmx}_update_guest_cr(), so that no flushes occur when
> the bit was set.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> Reported-by: Bitweasil <bitweasil@xxxxxxxxxxxxxx>
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> ---
> Changes since V1:
>  - Added the bool noflush parameter and code to propagate it to
>    {svm,vmx}_update_guest_cr().
>  - Added X86_CR3_NOFLUSH_DISABLE_MASK and X86_CR3_NOFLUSH_DISABLE.
>  - No longer sanitizing the old value in hvm_monitor_cr().
> ---
>  xen/arch/x86/hvm/domain.c         |  6 +++---
>  xen/arch/x86/hvm/hvm.c            | 25 ++++++++++++++++---------
>  xen/arch/x86/hvm/monitor.c        |  3 +++
>  xen/arch/x86/hvm/svm/nestedsvm.c  |  4 ++--
>  xen/arch/x86/hvm/svm/svm.c        | 22 ++++++++++++++--------
>  xen/arch/x86/hvm/svm/vmcb.c       |  4 ++--
>  xen/arch/x86/hvm/vmx/vmcs.c       |  4 ++--
>  xen/arch/x86/hvm/vmx/vmx.c        | 16 +++++++++-------
>  xen/arch/x86/mm.c                 |  2 +-
>  xen/arch/x86/mm/hap/hap.c         |  6 +++---
>  xen/arch/x86/mm/shadow/common.c   |  2 +-
>  xen/arch/x86/mm/shadow/multi.c    |  6 +++---
>  xen/arch/x86/mm/shadow/none.c     |  2 +-
>  xen/arch/x86/monitor.c            |  2 +-
>  xen/include/asm-x86/hvm/hvm.h     | 10 +++++++---
>  xen/include/asm-x86/hvm/svm/svm.h |  2 +-
>  xen/include/asm-x86/paging.h      |  7 ++++---
>  17 files changed, 73 insertions(+), 50 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> index 6047464..9be085e 100644
> --- a/xen/arch/x86/hvm/domain.c
> +++ b/xen/arch/x86/hvm/domain.c
> @@ -287,9 +287,9 @@ int arch_set_info_hvm_guest(struct vcpu *v, const 
> vcpu_hvm_context_t *ctx)
>          return -EINVAL;
>      }
>
> -    hvm_update_guest_cr(v, 0);
> -    hvm_update_guest_cr(v, 3);
> -    hvm_update_guest_cr(v, 4);
> +    hvm_update_guest_cr(v, 0, false);
> +    hvm_update_guest_cr(v, 3, false);
> +    hvm_update_guest_cr(v, 4, false);
>      hvm_update_guest_efer(v);
>
>      if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) )
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index c4287a3..b42fbd1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2184,7 +2184,7 @@ static void hvm_update_cr(struct vcpu *v, unsigned int 
> cr, unsigned long value)
>  {
>      v->arch.hvm_vcpu.guest_cr[cr] = value;
>      nestedhvm_set_cr(v, cr, value);
> -    hvm_update_guest_cr(v, cr);
> +    hvm_update_guest_cr(v, cr, false);
>  }
>
>  int hvm_set_cr0(unsigned long value, bool_t may_defer)
> @@ -2310,6 +2310,7 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
>      struct vcpu *v = current;
>      struct page_info *page;
>      unsigned long old = v->arch.hvm_vcpu.guest_cr[3];
> +    bool noflush = false;
>
>      if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled 
> &
>                                 monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3)) )

In this if block shouldn't we save the "noflush" into
"v->arch.vm_event->write_data" so that it can be used during
hvm_do_resume as well?

Tamas

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