[PATCH for-4.11] x86/SVM: Fix intercepted {RD, WR}MSR for the SYS{CALL, ENTER} MSRs

[PATCH for-4.11] x86/SVM: Fix intercepted {RD, WR}MSR for the SYS{CALL, ENTER} MSRs

[Andrew Cooper]

By default, the SYSCALL MSRs are not intercepted, and accesses are completed
by hardware.  The SYSENTER MSRs are intercepted for cross-vendor
purposes (albeit needlessly in the common case), and are fully emulated.

However, {RD,WR}MSR instructions which happen to be emulated (FEP,
introspection, or older versions of Xen which intercepted #UD), or when the
MSRs are explicitly intercepted (introspection), will be completed
incorrectly.

svm_msr_read_intercept() appears to return the correct values, but only
because of the default read-everything case (which is going to disappear), and
that in vcpu context, hardware should have the guest values in context.
Update the read path to explicitly sync the VMCB and complete the accesses,
rather than falling all the way through to the default case.

svm_msr_write_intercept() silently discard all updates.  Synchronise the VMCB
for all applicable MSRs, and implement suitable checks.  The actual behaviour
of AMD hardware is to truncate the SYSENTER and SFMASK MSRs at 32 bits, but
this isn't implemented yet to remain compatible with the cross-vendor case.

Drop one bit of trailing whitespace while modifing this area of the code.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Juergen Gross <jgross@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>

Juergen: As this patch probably wants backporting to the stable branches, it
probably wants to go into 4.11 at this point.

Outstanding TODOs (for someone with more tuits than me right now, or me when I
move this to the new MSR infrastructure):
 * Don't intercept the SYSENTER MSRs in the common case.
 * CPUID check for the long-mode only MSRs.
 * Proper cross-vendor behaviour.
---
 xen/arch/x86/hvm/svm/svm.c | 111 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 102 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c761645..2e42edf 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1883,6 +1883,22 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     switch ( msr )
     {
     case MSR_IA32_SYSENTER_CS:
+    case MSR_IA32_SYSENTER_ESP:
+    case MSR_IA32_SYSENTER_EIP:
+    case MSR_STAR:
+    case MSR_LSTAR:
+    case MSR_CSTAR:
+    case MSR_SYSCALL_MASK:
+    case MSR_FS_BASE:
+    case MSR_GS_BASE:
+    case MSR_SHADOW_GS_BASE:
+        svm_sync_vmcb(v);
+        break;
+    }
+
+    switch ( msr )
+    {
+    case MSR_IA32_SYSENTER_CS:
         *msr_content = v->arch.hvm_svm.guest_sysenter_cs;
         break;
     case MSR_IA32_SYSENTER_ESP:
@@ -1892,6 +1908,34 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         *msr_content = v->arch.hvm_svm.guest_sysenter_eip;
         break;
 
+    case MSR_STAR:
+        *msr_content = vmcb->star;
+        break;
+
+    case MSR_LSTAR:
+        *msr_content = vmcb->lstar;
+        break;
+
+    case MSR_CSTAR:
+        *msr_content = vmcb->cstar;
+        break;
+
+    case MSR_SYSCALL_MASK:
+        *msr_content = vmcb->sfmask;
+        break;
+
+    case MSR_FS_BASE:
+        *msr_content = vmcb->fs.base;
+        break;
+
+    case MSR_GS_BASE:
+        *msr_content = vmcb->gs.base;
+        break;
+
+    case MSR_SHADOW_GS_BASE:
+        *msr_content = vmcb->kerngsbase;
+        break;
+
     case MSR_IA32_MCx_MISC(4): /* Threshold register */
     case MSR_F10_MC4_MISC1 ... MSR_F10_MC4_MISC3:
         /*
@@ -2020,32 +2064,81 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
     int ret, result = X86EMUL_OKAY;
     struct vcpu *v = current;
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
-    int sync = 0;
+    bool sync = false;
 
     switch ( msr )
     {
     case MSR_IA32_SYSENTER_CS:
     case MSR_IA32_SYSENTER_ESP:
     case MSR_IA32_SYSENTER_EIP:
-        sync = 1;
-        break;
-    default:
+    case MSR_STAR:
+    case MSR_LSTAR:
+    case MSR_CSTAR:
+    case MSR_SYSCALL_MASK:
+    case MSR_FS_BASE:
+    case MSR_GS_BASE:
+    case MSR_SHADOW_GS_BASE:
+        sync = true;
         break;
     }
 
     if ( sync )
-        svm_sync_vmcb(v);    
+        svm_sync_vmcb(v);
 
     switch ( msr )
     {
+    case MSR_IA32_SYSENTER_ESP:
+    case MSR_IA32_SYSENTER_EIP:
+    case MSR_LSTAR:
+    case MSR_CSTAR:
+    case MSR_FS_BASE:
+    case MSR_GS_BASE:
+    case MSR_SHADOW_GS_BASE:
+        if ( !is_canonical_address(msr_content) )
+            goto gpf;
+
+        switch ( msr )
+        {
+        case MSR_IA32_SYSENTER_ESP:
+            vmcb->sysenter_esp = v->arch.hvm_svm.guest_sysenter_esp = msr_content;
+            break;
+
+        case MSR_IA32_SYSENTER_EIP:
+            vmcb->sysenter_eip = v->arch.hvm_svm.guest_sysenter_eip = msr_content;
+            break;
+
+        case MSR_LSTAR:
+            vmcb->lstar = msr_content;
+            break;
+
+        case MSR_CSTAR:
+            vmcb->cstar = msr_content;
+            break;
+
+        case MSR_FS_BASE:
+            vmcb->fs.base = msr_content;
+            break;
+
+        case MSR_GS_BASE:
+            vmcb->gs.base = msr_content;
+            break;
+
+        case MSR_SHADOW_GS_BASE:
+            vmcb->kerngsbase = msr_content;
+            break;
+        }
+        break;
+
     case MSR_IA32_SYSENTER_CS:
         vmcb->sysenter_cs = v->arch.hvm_svm.guest_sysenter_cs = msr_content;
         break;
-    case MSR_IA32_SYSENTER_ESP:
-        vmcb->sysenter_esp = v->arch.hvm_svm.guest_sysenter_esp = msr_content;
+
+    case MSR_STAR:
+        vmcb->star = msr_content;
         break;
-    case MSR_IA32_SYSENTER_EIP:
-        vmcb->sysenter_eip = v->arch.hvm_svm.guest_sysenter_eip = msr_content;
+
+    case MSR_SYSCALL_MASK:
+        vmcb->sfmask = msr_content;
         break;
 
     case MSR_IA32_DEBUGCTLMSR:
-- 
2.1.4


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

Re: [PATCH for-4.11] x86/SVM: Fix intercepted {RD, WR}MSR for the SYS{CALL, ENTER} MSRs

[Boris Ostrovsky]

On 04/24/2018 02:51 PM, Andrew Cooper wrote:
> By default, the SYSCALL MSRs are not intercepted, and accesses are completed
> by hardware.  The SYSENTER MSRs are intercepted for cross-vendor
> purposes (albeit needlessly in the common case), and are fully emulated.
>
> However, {RD,WR}MSR instructions which happen to be emulated (FEP,
> introspection, or older versions of Xen which intercepted #UD), or when the
> MSRs are explicitly intercepted (introspection), will be completed
> incorrectly.
>
> svm_msr_read_intercept() appears to return the correct values, but only
> because of the default read-everything case (which is going to disappear), and
> that in vcpu context, hardware should have the guest values in context.
> Update the read path to explicitly sync the VMCB and complete the accesses,
> rather than falling all the way through to the default case.
>
> svm_msr_write_intercept() silently discard all updates.  Synchronise the VMCB
> for all applicable MSRs, and implement suitable checks.  The actual behaviour
> of AMD hardware is to truncate the SYSENTER and SFMASK MSRs at 32 bits, but
> this isn't implemented yet to remain compatible with the cross-vendor case.
>
> Drop one bit of trailing whitespace while modifing this area of the code.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>


Reviewed-by:  Boris Ostrovsky <boris.ostrovsky@oracle.com>


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

Re: [PATCH for-4.11] x86/SVM: Fix intercepted {RD, WR}MSR for the SYS{CALL, ENTER} MSRs

[Juergen Gross]

On 24/04/18 20:51, Andrew Cooper wrote:
> By default, the SYSCALL MSRs are not intercepted, and accesses are completed
> by hardware.  The SYSENTER MSRs are intercepted for cross-vendor
> purposes (albeit needlessly in the common case), and are fully emulated.
> 
> However, {RD,WR}MSR instructions which happen to be emulated (FEP,
> introspection, or older versions of Xen which intercepted #UD), or when the
> MSRs are explicitly intercepted (introspection), will be completed
> incorrectly.
> 
> svm_msr_read_intercept() appears to return the correct values, but only
> because of the default read-everything case (which is going to disappear), and
> that in vcpu context, hardware should have the guest values in context.
> Update the read path to explicitly sync the VMCB and complete the accesses,
> rather than falling all the way through to the default case.
> 
> svm_msr_write_intercept() silently discard all updates.  Synchronise the VMCB
> for all applicable MSRs, and implement suitable checks.  The actual behaviour
> of AMD hardware is to truncate the SYSENTER and SFMASK MSRs at 32 bits, but
> this isn't implemented yet to remain compatible with the cross-vendor case.
> 
> Drop one bit of trailing whitespace while modifing this area of the code.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>
> 
> Juergen: As this patch probably wants backporting to the stable branches, it
> probably wants to go into 4.11 at this point.

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

Re: [PATCH for-4.11] x86/SVM: Fix intercepted {RD, WR}MSR for the SYS{CALL, ENTER} MSRs

[Jan Beulich]

>>> On 24.04.18 at 20:51, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1883,6 +1883,22 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>      switch ( msr )
>      {
>      case MSR_IA32_SYSENTER_CS:
> +    case MSR_IA32_SYSENTER_ESP:
> +    case MSR_IA32_SYSENTER_EIP:

These three do not require sync-ing, as their values aren't read from the VMCB.
(They do require sync-ing on the write path).

I also don't think this is going to fully resolve Razvan's issue (not the least
because the code paths you adjust aren't involved in his scenario): As
pointed out in a private mail, I think vmcb_in_sync needs to start out as
true for a vCPU, and may need setting to true upon context set and/or
reset/init emulation.

Jan



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

Re: [PATCH for-4.11] x86/SVM: Fix intercepted {RD, WR}MSR for the SYS{CALL, ENTER} MSRs

[Razvan Cojocaru]

On 04/25/2018 09:49 AM, Jan Beulich wrote:
>>>> On 24.04.18 at 20:51, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1883,6 +1883,22 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>>       switch ( msr )
>>       {
>>       case MSR_IA32_SYSENTER_CS:
>> +    case MSR_IA32_SYSENTER_ESP:
>> +    case MSR_IA32_SYSENTER_EIP:
> 
> These three do not require sync-ing, as their values aren't read from the VMCB.
> (They do require sync-ing on the write path).
> 
> I also don't think this is going to fully resolve Razvan's issue (not the least
> because the code paths you adjust aren't involved in his scenario): As
> pointed out in a private mail, I think vmcb_in_sync needs to start out as
> true for a vCPU, and may need setting to true upon context set and/or
> reset/init emulation.

It indeed does not solve the whole issue - I've tested the patch as soon 
as it was posted (and I've tried a similar strategy for LSTAR alone as 
part of debugging before notifying of the issue). But I think Andrew 
meant it as a separate patch, fixing only the intercept part.

I'll test the vmcb_in_sync suggestion ASAP.


Thanks,
Razvan

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

Re: [PATCH for-4.11] x86/SVM: Fix intercepted {RD, WR}MSR for the SYS{CALL, ENTER} MSRs

[Andrew Cooper]

On 25/04/2018 07:49, Jan Beulich wrote:
>>>> On 24.04.18 at 20:51, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1883,6 +1883,22 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>>      switch ( msr )
>>      {
>>      case MSR_IA32_SYSENTER_CS:
>> +    case MSR_IA32_SYSENTER_ESP:
>> +    case MSR_IA32_SYSENTER_EIP:
> These three do not require sync-ing, as their values aren't read from the VMCB.
> (They do require sync-ing on the write path).

See the TODO list in the patch comment.  This is a quirk of cross-vendor
logic being used unnecessarily in the common case, and isn't going to
remain like this.

> I also don't think this is going to fully resolve Razvan's issue (not the least
> because the code paths you adjust aren't involved in his scenario): As
> pointed out in a private mail, I think vmcb_in_sync needs to start out as
> true for a vCPU, and may need setting to true upon context set and/or
> reset/init emulation.

No - it wont, but its obviously broken and will be the second bug to be
hit by introspection, when interception of these MSRs is active.

It just so happened that it was the easier issue to get started with.

~Andrew

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

Re: [PATCH for-4.11] x86/SVM: Fix intercepted {RD, WR}MSR for the SYS{CALL, ENTER} MSRs

[Razvan Cojocaru]

On 04/25/2018 09:49 AM, Jan Beulich wrote:
>>>> On 24.04.18 at 20:51, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1883,6 +1883,22 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>>      switch ( msr )
>>      {
>>      case MSR_IA32_SYSENTER_CS:
>> +    case MSR_IA32_SYSENTER_ESP:
>> +    case MSR_IA32_SYSENTER_EIP:
> 
> These three do not require sync-ing, as their values aren't read from the VMCB.
> (They do require sync-ing on the write path).
> 
> I also don't think this is going to fully resolve Razvan's issue (not the least
> because the code paths you adjust aren't involved in his scenario): As
> pointed out in a private mail, I think vmcb_in_sync needs to start out as
> true for a vCPU, and may need setting to true upon context set and/or
> reset/init emulation.

Doing arch_svm->vmcb_in_sync = 1; in construct_vmcb() does solve the
issue. I can't unfortunately test if it also needs setting in other
places as our internal patches still need some work so introspection is
not yet fully functional on SVM (mem_access events specifically are a
bit of a problem).


Thanks,
Razvan

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