[PATCH for-4.8] x86/svm: Fix svm_nextrip_insn_length() when crossing the virtual boundary to 0

[PATCH for-4.8] x86/svm: Fix svm_nextrip_insn_length() when crossing the virtual boundary to 0

[Andrew Cooper]

vmcb->nextrip can legitimately be less than vmcb->rip when execution wraps
back around to 0.  Instead, complain if the reported length is greater than 15
and use x86_decode_insn() as a fallback.

While making changes here, fix two whitespace issues with the case labels.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/svm/emulate.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index 36585b0..ca670bf 100644
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -30,18 +30,18 @@ static unsigned long svm_nextrip_insn_length(struct vcpu *v)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
 
-    if ( !cpu_has_svm_nrips || (vmcb->nextrip <= vmcb->rip) )
+    if ( !cpu_has_svm_nrips )
         return 0;
 
 #ifndef NDEBUG
     switch ( vmcb->exitcode )
     {
-    case VMEXIT_CR0_READ... VMEXIT_DR15_WRITE:
+    case VMEXIT_CR0_READ ... VMEXIT_DR15_WRITE:
         /* faults due to instruction intercepts */
         /* (exitcodes 84-95) are reserved */
     case VMEXIT_IDTR_READ ... VMEXIT_TR_WRITE:
     case VMEXIT_RDTSC ... VMEXIT_MSR:
-    case VMEXIT_VMRUN ...  VMEXIT_XSETBV:
+    case VMEXIT_VMRUN ... VMEXIT_XSETBV:
         /* ...and the rest of the #VMEXITs */
     case VMEXIT_CR0_SEL_WRITE:
     case VMEXIT_EXCEPTION_BP:
@@ -88,7 +88,8 @@ int __get_instruction_length_from_list(struct vcpu *v,
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     struct hvm_emulate_ctxt ctxt;
     struct x86_emulate_state *state;
-    unsigned int inst_len, j, modrm_rm, modrm_reg;
+    unsigned long inst_len, j;
+    unsigned int modrm_rm, modrm_reg;
     int modrm_mod;
 
     /*
@@ -96,7 +97,9 @@ int __get_instruction_length_from_list(struct vcpu *v,
      * hardware.
      */
 #ifdef NDEBUG
-    if ( (inst_len = svm_nextrip_insn_length(v)) != 0 )
+    if ( (inst_len = svm_nextrip_insn_length(v)) > MAX_INST_LEN )
+        gprintk(XENLOG_WARNING, "NRip reported insn_len %lu\n", insn_len);
+    else if ( insn_len != 0 )
         return inst_len;
 
     if ( vmcb->exitcode == VMEXIT_IOIO )
@@ -120,7 +123,7 @@ int __get_instruction_length_from_list(struct vcpu *v,
         j = svm_nextrip_insn_length(v);
     if ( j && j != inst_len )
     {
-        gprintk(XENLOG_WARNING, "insn-len[%02x]=%u (exp %u)\n",
+        gprintk(XENLOG_WARNING, "insn-len[%02x]=%lu (exp %lu)\n",
                 ctxt.ctxt.opcode, inst_len, j);
         return j;
     }
-- 
2.1.4


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

Re: [PATCH for-4.8] x86/svm: Fix svm_nextrip_insn_length() when crossing the virtual boundary to 0

[Jan Beulich]

>>> On 16.11.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> vmcb->nextrip can legitimately be less than vmcb->rip when execution wraps
> back around to 0.  Instead, complain if the reported length is greater than 15
> and use x86_decode_insn() as a fallback.
> 
> While making changes here, fix two whitespace issues with the case labels.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH for-4.8] x86/svm: Fix svm_nextrip_insn_length() when crossing the virtual boundary to 0

[Wei Liu]

On Wed, Nov 16, 2016 at 03:56:04AM -0700, Jan Beulich wrote:
> >>> On 16.11.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> > vmcb->nextrip can legitimately be less than vmcb->rip when execution wraps
> > back around to 0.  Instead, complain if the reported length is greater than 15
> > and use x86_decode_insn() as a fallback.
> > 
> > While making changes here, fix two whitespace issues with the case labels.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH for-4.8] x86/svm: Fix svm_nextrip_insn_length() when crossing the virtual boundary to 0

[Andrew Cooper]

On 16/11/16 10:51, Andrew Cooper wrote:
> vmcb->nextrip can legitimately be less than vmcb->rip when execution wraps
> back around to 0.  Instead, complain if the reported length is greater than 15
> and use x86_decode_insn() as a fallback.
>
> While making changes here, fix two whitespace issues with the case labels.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Wei Liu <wei.liu2@citrix.com>

SVM maintainers, ping?

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

Re: [PATCH for-4.8] x86/svm: Fix svm_nextrip_insn_length() when crossing the virtual boundary to 0

[Boris Ostrovsky]

On 11/21/2016 05:40 AM, Andrew Cooper wrote:
> On 16/11/16 10:51, Andrew Cooper wrote:
>> vmcb->nextrip can legitimately be less than vmcb->rip when execution wraps
>> back around to 0.  Instead, complain if the reported length is greater than 15
>> and use x86_decode_insn() as a fallback.

Why do we need to complain? In the case that you are addressing by this
patch wouldn't that be the expected result (length>15)?

-boris


>>
>> While making changes here, fix two whitespace issues with the case labels.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
> SVM maintainers, ping?


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

Re: [PATCH for-4.8] x86/svm: Fix svm_nextrip_insn_length() when crossing the virtual boundary to 0

[Andrew Cooper]

On 21/11/16 13:38, Boris Ostrovsky wrote:
> On 11/21/2016 05:40 AM, Andrew Cooper wrote:
>> On 16/11/16 10:51, Andrew Cooper wrote:
>>> vmcb->nextrip can legitimately be less than vmcb->rip when execution wraps
>>> back around to 0.  Instead, complain if the reported length is greater than 15
>>> and use x86_decode_insn() as a fallback.
> Why do we need to complain? In the case that you are addressing by this
> patch wouldn't that be the expected result (length>15)?

No.  An instruction crossing the boundary looks like:

e.g. nextrip = 0x3, rip = 0xfffffffffffffffe

As this is all evaluated in unsigned long arithmetic, nextrip - rip
evaluates to 5, which is correct.

~Andrew

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

Re: [PATCH for-4.8] x86/svm: Fix svm_nextrip_insn_length() when crossing the virtual boundary to 0

[Boris Ostrovsky]

On 11/21/2016 08:53 AM, Andrew Cooper wrote:
> On 21/11/16 13:38, Boris Ostrovsky wrote:
>> On 11/21/2016 05:40 AM, Andrew Cooper wrote:
>>> On 16/11/16 10:51, Andrew Cooper wrote:
>>>> vmcb->nextrip can legitimately be less than vmcb->rip when execution wraps
>>>> back around to 0.  Instead, complain if the reported length is greater than 15
>>>> and use x86_decode_insn() as a fallback.
>> Why do we need to complain? In the case that you are addressing by this
>> patch wouldn't that be the expected result (length>15)?
> No.  An instruction crossing the boundary looks like:
>
> e.g. nextrip = 0x3, rip = 0xfffffffffffffffe
>
> As this is all evaluated in unsigned long arithmetic, nextrip - rip
> evaluates to 5, which is correct.

Oh, right.

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

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