]> xenbits.xensource.com Git - people/vhanquez/xen.git/commitdiff
SVM: clean up __get_instruction_length_from_list()
authorKeir Fraser <keir.fraser@citrix.com>
Tue, 13 May 2008 14:54:31 +0000 (15:54 +0100)
committerKeir Fraser <keir.fraser@citrix.com>
Tue, 13 May 2008 14:54:31 +0000 (15:54 +0100)
Remove unused arguments, fix its behaviour near page boundaries,
inject appropriate pagefaults, and inject #GP if the instruction is
not decodable or %eip is not pointing to valid RAM.

Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>
Signed-off-by: Keir Fraser <keir.fraser@citrix.com>
xen-unstable changeset:   17575:01aa7c088e983cd54b61faeb3ff533581714a26f
xen-unstable date:        Tue May 06 13:32:18 2008 +0100

xen/arch/x86/hvm/svm/emulate.c
xen/arch/x86/hvm/svm/svm.c
xen/arch/x86/hvm/vmx/vmx.c

index e00ffbf377dfea880de0aa887079069b89ae7475..373a590d1420ed4d2172d5f5c94faae09dc9e918 100644 (file)
@@ -29,9 +29,6 @@
 #include <asm/hvm/svm/emulate.h>
 
 
-extern int inst_copy_from_guest(unsigned char *buf, unsigned long guest_eip,
-        int inst_len);
-
 #define REX_PREFIX_BASE 0x40
 #define REX_X           0x02
 #define REX_W           0x08
@@ -412,8 +409,8 @@ static const u8 *opc_bytes[INSTR_MAX_COUNT] =
  * Intel has a vmcs entry to give the instruction length. AMD doesn't.  So we
  * have to do a little bit of work to find out... 
  *
- * The caller can either pass a NULL pointer to the guest_eip_buf, or a pointer
- * to enough bytes to satisfy the instruction including prefix bytes.
+ * The caller may supply a buffer of at least MAX_INST_LEN bytes, which
+ * the instruction will be read into.
  */
 int __get_instruction_length_from_list(struct vcpu *v,
         enum instruction_index *list, unsigned int list_count, 
@@ -425,21 +422,40 @@ int __get_instruction_length_from_list(struct vcpu *v,
     unsigned int j;
     int found = 0;
     enum instruction_index instr = 0;
-    u8 buffer[MAX_INST_LEN];
+    unsigned long fetch_addr;
+    int fetch_len;
     u8 *buf;
     const u8 *opcode = NULL;
-
-    if (guest_eip_buf)
-    {
-        buf = guest_eip_buf;
-    }
-    else
-    {
-        if ( inst_copy_from_guest(buffer, svm_rip2pointer(v), MAX_INST_LEN)
-             != MAX_INST_LEN )
-            return 0;
-        buf = buffer;
-    }
+    u8 temp_buffer[MAX_INST_LEN];
+
+    /* Use the stack if the caller didn't give us a buffer */
+    buf = ( guest_eip_buf ) ? guest_eip_buf : temp_buffer;
+
+#define FETCH(_buf, _addr, _len) do                                           \
+    {                                                                         \
+        switch ( hvm_fetch_from_guest_virt((_buf), (_addr), (_len)) )         \
+        {                                                                     \
+        case HVMCOPY_okay:                                                    \
+            break;                                                            \
+        case HVMCOPY_bad_gva_to_gfn:                                          \
+            /* OK just to give up; we'll have injected #PF already */         \
+            return 0;                                                         \
+        case HVMCOPY_bad_gfn_to_mfn:                                          \
+            /* Not OK: fetches from non-RAM pages are not supportable. */     \
+            gdprintk(XENLOG_ERR, "Bad instruction fetch at %#lx (%#lx)\n",    \
+                     (unsigned long) guest_cpu_user_regs()->eip, fetch_addr); \
+            hvm_inject_exception(TRAP_gp_fault, 0, 0);                        \
+            return 0;                                                         \
+        }                                                                     \
+    } while (0)
+
+    /* Fetch up to the next page break; we'll fetch from the next page
+     * later if we have to. */
+    fetch_addr = svm_rip2pointer(v);
+    fetch_len = PAGE_SIZE - (fetch_addr & ~PAGE_MASK) ;
+    if ( fetch_len > MAX_INST_LEN ) 
+        fetch_len = MAX_INST_LEN;
+    FETCH(buf, fetch_addr, fetch_len);
 
     for (j = 0; j < list_count; j++)
     {
@@ -450,7 +466,16 @@ int __get_instruction_length_from_list(struct vcpu *v,
         while (inst_len < MAX_INST_LEN && 
                 is_prefix(buf[inst_len]) && 
                 !is_prefix(opcode[1]))
+        {
             inst_len++;
+            if ( inst_len >= fetch_len ) 
+            { 
+                FETCH(buf + fetch_len, 
+                      fetch_addr + fetch_len, 
+                      MAX_INST_LEN - fetch_len);
+                fetch_len = MAX_INST_LEN;
+            }
+        }
 
         ASSERT(opcode[0] <= 15);    /* Make sure the table is correct. */
         found = 1;
@@ -461,6 +486,14 @@ int __get_instruction_length_from_list(struct vcpu *v,
             if (i == opcode[0]-1 && opcode[i+1] == 0)
                 break;
 
+            if ( inst_len + i >= fetch_len ) 
+            { 
+                FETCH(buf + fetch_len, 
+                      fetch_addr + fetch_len, 
+                      MAX_INST_LEN - fetch_len);
+                fetch_len = MAX_INST_LEN;
+            }
+
             if (buf[inst_len+i] != opcode[i+1])
             {
                 found = 0;
@@ -487,7 +520,11 @@ int __get_instruction_length_from_list(struct vcpu *v,
 
     printk("%s: Mismatch between expected and actual instruction bytes: "
             "eip = %lx\n",  __func__, (unsigned long)vmcb->rip);
+    hvm_inject_exception(TRAP_gp_fault, 0, 0);
     return 0;
+
+#undef FETCH
+
 }
 
 /*
index d83172e21d9a2bad72337c047eac5b900d36d0fd..cde129d24c286b5042b76a5230b2623bd4f56b76 100644 (file)
@@ -78,7 +78,10 @@ static void inline __update_guest_eip(
 {
     struct vcpu *curr = current;
 
-    if ( unlikely((inst_len == 0) || (inst_len > 15)) )
+    if ( unlikely(inst_len == 0) )
+        return;
+
+    if ( unlikely(inst_len > 15) )
     {
         gdprintk(XENLOG_ERR, "Bad instruction length %u\n", inst_len);
         domain_crash(curr->domain);
@@ -1131,18 +1134,28 @@ static void svm_dr_access(struct vcpu *v, struct cpu_user_regs *regs)
 
 static int svm_get_prefix_info(struct vcpu *v, unsigned int dir, 
                                 svm_segment_register_t **seg, 
-                                unsigned int *asize)
+                                unsigned int *asize,
+                                unsigned int isize)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     unsigned char inst[MAX_INST_LEN];
     int i;
 
     memset(inst, 0, MAX_INST_LEN);
-    if (inst_copy_from_guest(inst, svm_rip2pointer(v), sizeof(inst)) 
-        != MAX_INST_LEN) 
+    
+    switch ( hvm_fetch_from_guest_virt(inst, svm_rip2pointer(v), isize) )
     {
-        gdprintk(XENLOG_ERR, "get guest instruction failed\n");
-        return 0;
+        case HVMCOPY_okay:
+            break;
+        case HVMCOPY_bad_gva_to_gfn:
+            /* OK just to give up; we'll have injected #PF already */
+            return 0;
+        case HVMCOPY_bad_gfn_to_mfn:
+            gdprintk(XENLOG_ERR, "Bad prefix fetch at %#lx (%#lx)\n",
+                     (unsigned long) guest_cpu_user_regs()->eip,
+                     svm_rip2pointer(v));
+            domain_crash(v->domain);
+            return 0;
     }
 
     for (i = 0; i < MAX_INST_LEN; i++)
@@ -1232,12 +1245,8 @@ static int svm_get_io_address(
      * to figure out what it is...
      */
     isize = vmcb->exitinfo2 - regs->eip;
-
-    if (info.fields.rep)
-        isize --;
-
-    if (isize > 1) 
-        if ( !svm_get_prefix_info(v, info.fields.type, &seg, &asize) )
+    if ( isize > ((info.fields.rep) ? 2 : 1) ) 
+        if ( !svm_get_prefix_info(v, info.fields.type, &seg, &asize, isize) )
             return 0;
 
     if (info.fields.type == IOREQ_WRITE)
@@ -1593,30 +1602,24 @@ static void svm_cr_access(
     enum instruction_index list_b[] = {INSTR_MOVCR2, INSTR_SMSW};
     enum instruction_index match;
 
-    if ( inst_copy_from_guest(buffer, svm_rip2pointer(v), sizeof(buffer))
-         != sizeof buffer )
-        /* #PF will have been delivered if appropriate. */
-        return;
 
-    /* get index to first actual instruction byte - as we will need to know 
-       where the prefix lives later on */
-    index = skip_prefix_bytes(buffer, sizeof(buffer));
-    
     if ( type == TYPE_MOV_TO_CR )
     {
         inst_len = __get_instruction_length_from_list(
-            v, list_a, ARRAY_SIZE(list_a), &buffer[index], &match);
+            v, list_a, ARRAY_SIZE(list_a), buffer, &match);
     }
     else /* type == TYPE_MOV_FROM_CR */
     {
         inst_len = __get_instruction_length_from_list(
-            v, list_b, ARRAY_SIZE(list_b), &buffer[index], &match);
+            v, list_b, ARRAY_SIZE(list_b), buffer, &match);
     }
 
     if ( inst_len == 0 )
         return;
 
-    inst_len += index;
+    /* get index to first actual instruction byte - as we will need to know 
+       where the prefix lives later on */
+    index = skip_prefix_bytes(buffer, sizeof(buffer));
 
     /* Check for REX prefix - it's ALWAYS the last byte of any prefix bytes */
     if (index > 0 && (buffer[index-1] & 0xF0) == 0x40)
@@ -1941,16 +1944,6 @@ void svm_handle_invlpg(const short invlpga, struct cpu_user_regs *regs)
     unsigned long g_vaddr;
     int inst_len;
 
-    /* 
-     * Unknown how many bytes the invlpg instruction will take.  Use the
-     * maximum instruction length here
-     */
-    if ( inst_copy_from_guest(opcode, svm_rip2pointer(v), length) < length )
-    {
-        gdprintk(XENLOG_ERR, "Error reading memory %d bytes\n", length);
-        return;
-    }
-
     if ( invlpga )
     {
         inst_len = __get_instruction_length(v, INSTR_INVLPGA, opcode);
@@ -1965,8 +1958,8 @@ void svm_handle_invlpg(const short invlpga, struct cpu_user_regs *regs)
     else
     {
         /* What about multiple prefix codes? */
-        prefix = (is_prefix(opcode[0]) ? opcode[0] : 0);
         inst_len = __get_instruction_length(v, INSTR_INVLPG, opcode);
+        prefix = (is_prefix(opcode[0]) ? opcode[0] : 0);
         if ( inst_len <= 0 )
         {
             gdprintk(XENLOG_ERR, "Error getting invlpg instr len\n");
index 67fe4ccd278a051bb2b45528590887b6b5cb0a11..475efff47ad88cd9ce245edd0388b4d2a70f7305 100644 (file)
@@ -1390,7 +1390,6 @@ static enum x86_segment vmx_outs_get_segment(
     unsigned char inst[MAX_INST_LEN];
     enum x86_segment seg = x86_seg_ds;
     int i;
-    extern int inst_copy_from_guest(unsigned char *, unsigned long, int);
 
     if ( likely(cpu_has_vmx_ins_outs_instr_info) )
     {
@@ -1415,7 +1414,7 @@ static enum x86_segment vmx_outs_get_segment(
         eip += __vmread(GUEST_CS_BASE);
 
     memset(inst, 0, MAX_INST_LEN);
-    if ( inst_copy_from_guest(inst, eip, inst_len) != inst_len )
+    if ( hvm_fetch_from_guest_virt(inst, eip, inst_len) != HVMCOPY_okay )
     {
         gdprintk(XENLOG_ERR, "Get guest instruction failed\n");
         domain_crash(current->domain);