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

2018-02-08 Thread Razvan Cojocaru 
On 02/07/2018 07:42 PM, Razvan Cojocaru wrote:
> On 02/07/2018 07:01 PM, Jan Beulich wrote:
>>> --- a/xen/include/asm-x86/hvm/hvm.h
>>> +++ b/xen/include/asm-x86/hvm/hvm.h
>>> @@ -34,6 +34,9 @@ extern bool_t opt_hvm_fep;
>>>  #define opt_hvm_fep 0
>>>  #endif
>>>  
>>> +#define X86_CR3_NOFLUSH (1ull << 63)
>>
>> This belongs in x86-defs.h

Sorry, do you mean xen/arch/x86/boot/defs.h? Or that I should add a new
header for this?

'find . -name x86-defs.h' comes up empty-handed.


Thanks,
Razvan

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

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

2018-02-07 Thread Razvan Cojocaru 
On 02/07/2018 07:01 PM, Jan Beulich wrote:
 On 02.02.18 at 09:14,  wrote:
>> @@ -2313,6 +2314,12 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
>>  }
>>  }
>>  
>> +if ( hvm_pcid_enabled(v) ) /* Clear the noflush bit. */
>> +{
>> +noflush = !!(value & X86_CR3_NOFLUSH);
> 
> Pointless !!.

I'll change it.

>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -34,6 +34,9 @@ extern bool_t opt_hvm_fep;
>>  #define opt_hvm_fep 0
>>  #endif
>>  
>> +#define X86_CR3_NOFLUSH (1ull << 63)
> 
> This belongs in x86-defs.h

I'll move it.

>> +#define X86_CR3_NOFLUSH_DISABLE_MASK (X86_CR3_NOFLUSH - 1)
> 
> This shouldn't be needed (use ~X86_CR3_NOFLUSH instead).

Agreed.

>> @@ -322,9 +325,10 @@ hvm_update_host_cr3(struct vcpu *v)
>>  hvm_funcs.update_host_cr3(v);
>>  }
>>  
>> -static inline void hvm_update_guest_cr(struct vcpu *v, unsigned int cr)
>> +static inline void hvm_update_guest_cr(struct vcpu *v, unsigned int cr,
>> +   bool noflush)
>>  {
>> -hvm_funcs.update_guest_cr(v, cr);
>> +hvm_funcs.update_guest_cr(v, cr, noflush);
>>  }
> 
> Instead of altering this function (and a lot of callers), how about
> introducing
> 
> static inline void hvm_update_guest_cr3(struct vcpu *v, bool noflush)
> {
> hvm_funcs.update_guest_cr(v, 3, noflush);
> }
> 
> ? I'm also not convinced of the update_guest_cr() hook taking a
> bool which is meaningless for all other CRs. Perhaps a more general
> flags parameter would be better, with CR-specific meaning (you'd
> then e.g. introduce HVM_UPDATE_GUEST_CR3_NO_FLUSH).

Very true. I'll change the patch.


Thanks for the review,
Razvan

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

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

2018-02-07 Thread Jan Beulich 
>>> On 02.02.18 at 09:14,  wrote:
> @@ -2313,6 +2314,12 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
>  }
>  }
>  
> +if ( hvm_pcid_enabled(v) ) /* Clear the noflush bit. */
> +{
> +noflush = !!(value & X86_CR3_NOFLUSH);

Pointless !!.

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -34,6 +34,9 @@ extern bool_t opt_hvm_fep;
>  #define opt_hvm_fep 0
>  #endif
>  
> +#define X86_CR3_NOFLUSH (1ull << 63)

This belongs in x86-defs.h

> +#define X86_CR3_NOFLUSH_DISABLE_MASK (X86_CR3_NOFLUSH - 1)

This shouldn't be needed (use ~X86_CR3_NOFLUSH instead).

> @@ -322,9 +325,10 @@ hvm_update_host_cr3(struct vcpu *v)
>  hvm_funcs.update_host_cr3(v);
>  }
>  
> -static inline void hvm_update_guest_cr(struct vcpu *v, unsigned int cr)
> +static inline void hvm_update_guest_cr(struct vcpu *v, unsigned int cr,
> +   bool noflush)
>  {
> -hvm_funcs.update_guest_cr(v, cr);
> +hvm_funcs.update_guest_cr(v, cr, noflush);
>  }

Instead of altering this function (and a lot of callers), how about
introducing

static inline void hvm_update_guest_cr3(struct vcpu *v, bool noflush)
{
hvm_funcs.update_guest_cr(v, 3, noflush);
}

? I'm also not convinced of the update_guest_cr() hook taking a
bool which is meaningless for all other CRs. Perhaps a more general
flags parameter would be better, with CR-specific meaning (you'd
then e.g. introduce HVM_UPDATE_GUEST_CR3_NO_FLUSH).

Jan


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

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

2018-02-02 Thread Tamas K Lengyel 
On Fri, Feb 2, 2018 at 1:14 AM, Razvan Cojocaru
 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 
> Reported-by: Bitweasil 
> Suggested-by: Andrew Cooper 

Acked-by: Tamas K Lengyel 

>
> ---
> Changes since V2:
>  - Fixed the write_ctrlreg.index check (the proper comparison
>is with VM_EVENT_X86_CR3, not 3). Noticed by Tamas.
> ---
>  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 18d721d..10c62fb 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2171,7 +2171,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)
> @@ -2297,6 +2297,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)) )
> @@ -2313,6 +2314,12 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
>  }
>  }
>
> +if ( hvm_pcid_enabled(v) ) /* Clear the noflush bit. */
> +{
> +noflush = !!(value & X86_CR3_NOFLUSH);
> +value &= X86_CR3_NOFLUSH_DISABLE_MASK;
> +}
> +
>  if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) &&
>   (value != v->arch.hvm_vcpu.guest_cr[3]) )
>  {
> @@ -2330,7 +2337,7 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
>  }
>
>  v->arch.hvm_vcpu.guest_cr[3] = value;
> -paging_update_cr3(v);
> +paging_update_cr3(v, noflush);
>  return X86EMUL_OKAY;
>
>   bad_cr3:
> @@ -3059,7 +3066,7 @@ void hvm_task_switch(
>  hvm_set_segment_register(v, x86_seg_tr, );
>
>  v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_TS;
> -hvm_update_guest_cr(v, 0);
> +hvm_update_guest_cr(v, 0, false);
>
>  if ( (taskswitch_reason == TSW_iret ||
>taskswitch_reason == TSW_jmp) && otd_writable )
> @@ -3895,16 +3902,16 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t 
> cs, uint16_t ip)
>  memset(>arch.debugreg, 0, sizeof(v->arch.debugreg));
>
>  v->arch.hvm_vcpu.guest_cr[0] = X86_CR0_ET;
> -hvm_update_guest_cr(v, 0);
> +hvm_update_guest_cr(v, 0, false);
>
>  v->arch.hvm_vcpu.guest_cr[2] = 0;
> -hvm_update_guest_cr(v, 2);
> +hvm_update_guest_cr(v, 2, false);
>
>  v->arch.hvm_vcpu.guest_cr[3] = 0;
> -hvm_update_guest_cr(v, 3);
> +hvm_update_guest_cr(v, 3, false);
>
>  v->arch.hvm_vcpu.guest_cr[4] = 0;
> -

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

2018-02-02 Thread Razvan Cojocaru 
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 
Reported-by: Bitweasil 
Suggested-by: Andrew Cooper 

---
Changes since V2:
 - Fixed the write_ctrlreg.index check (the proper comparison
   is with VM_EVENT_X86_CR3, not 3). Noticed by Tamas.
---
 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 18d721d..10c62fb 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2171,7 +2171,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)
@@ -2297,6 +2297,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)) )
@@ -2313,6 +2314,12 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
 }
 }
 
+if ( hvm_pcid_enabled(v) ) /* Clear the noflush bit. */
+{
+noflush = !!(value & X86_CR3_NOFLUSH);
+value &= X86_CR3_NOFLUSH_DISABLE_MASK;
+}
+
 if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) &&
  (value != v->arch.hvm_vcpu.guest_cr[3]) )
 {
@@ -2330,7 +2337,7 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
 }
 
 v->arch.hvm_vcpu.guest_cr[3] = value;
-paging_update_cr3(v);
+paging_update_cr3(v, noflush);
 return X86EMUL_OKAY;
 
  bad_cr3:
@@ -3059,7 +3066,7 @@ void hvm_task_switch(
 hvm_set_segment_register(v, x86_seg_tr, );
 
 v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_TS;
-hvm_update_guest_cr(v, 0);
+hvm_update_guest_cr(v, 0, false);
 
 if ( (taskswitch_reason == TSW_iret ||
   taskswitch_reason == TSW_jmp) && otd_writable )
@@ -3895,16 +3902,16 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, 
uint16_t ip)
 memset(>arch.debugreg, 0, sizeof(v->arch.debugreg));
 
 v->arch.hvm_vcpu.guest_cr[0] = X86_CR0_ET;
-hvm_update_guest_cr(v, 0);
+hvm_update_guest_cr(v, 0, false);
 
 v->arch.hvm_vcpu.guest_cr[2] = 0;
-hvm_update_guest_cr(v, 2);
+hvm_update_guest_cr(v, 2, false);
 
 v->arch.hvm_vcpu.guest_cr[3] = 0;
-hvm_update_guest_cr(v, 3);
+hvm_update_guest_cr(v, 3, false);
 
 v->arch.hvm_vcpu.guest_cr[4] = 0;
-hvm_update_guest_cr(v, 4);
+hvm_update_guest_cr(v, 4, false);
 
 v->arch.hvm_vcpu.guest_efer = 0;
 hvm_update_guest_efer(v);
@@ -4031,7 +4038,7 @@ static int hvmop_flush_tlb_all(void)
 
 /* Flush paging-mode soft state (e.g., va->gfn cache; PAE PDPE cache). */
 for_each_vcpu ( d, v )
-paging_update_cr3(v);
+paging_update_cr3(v,