Tim Deegan
2008-May-02 15:32 UTC
[Xen-devel] [PATCH] Clean up SVM instruction-fetch code some more
SVM: clean up __get_instruction_length_from_list()
Remove unused arguments, fix its behaviour near page boundaries,
inject appropriate pagefaults, and kill the guest only if the
instruction is not decodable or %eip is not pointing to valid RAM.
Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>
diff -r 483d006cc607 -r c20b8931b598 xen/arch/x86/hvm/svm/emulate.c
--- a/xen/arch/x86/hvm/svm/emulate.c Fri Apr 25 13:46:27 2008 +0100
+++ b/xen/arch/x86/hvm/svm/emulate.c Fri May 02 16:28:05 2008 +0100
@@ -28,18 +28,6 @@
#include <asm/hvm/svm/emulate.h>
#define MAX_INST_LEN 15
-
-static int inst_copy_from_guest(
- unsigned char *buf, unsigned long guest_eip, int inst_len)
-{
- struct vmcb_struct *vmcb = current->arch.hvm_svm.vmcb;
- uint32_t pfec = (vmcb->cpl == 3) ? PFEC_user_mode : 0;
- if ( (inst_len > MAX_INST_LEN) || (inst_len <= 0) )
- return 0;
- if ( hvm_fetch_from_guest_virt_nofault(buf, guest_eip, inst_len, pfec) )
- return 0;
- return inst_len;
-}
static unsigned int is_prefix(u8 opc)
{
@@ -101,29 +89,47 @@ static const u8 *opc_bytes[INSTR_MAX_COU
[INSTR_INT3] = OPCODE_INT3
};
+static int fetch(struct vcpu *v, u8 *buf, unsigned long addr, int len)
+{
+ uint32_t pfec = (v->arch.hvm_svm.vmcb->cpl == 3) ? PFEC_user_mode :
0;
+
+ switch ( hvm_fetch_from_guest_virt(buf, addr, len, pfec) )
+ {
+ case HVMCOPY_okay:
+ return 1;
+ 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:
+ default:
+ /* 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, addr);
+ domain_crash(v->domain);
+ return 0;
+ }
+}
+
int __get_instruction_length_from_list(struct vcpu *v,
- enum instruction_index *list, unsigned int list_count,
- u8 *guest_eip_buf, enum instruction_index *match)
+ enum instruction_index *list, unsigned int list_count)
{
struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
unsigned int i, j, inst_len = 0;
int found = 0;
enum instruction_index instr = 0;
- u8 buffer[MAX_INST_LEN];
- u8 *buf;
+ u8 buf[MAX_INST_LEN];
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;
- }
+ unsigned long fetch_addr;
+ int fetch_len;
+
+ /* 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;
+ if ( !fetch(v, buf, fetch_addr, fetch_len) )
+ return 0;
for ( j = 0; j < list_count; j++ )
{
@@ -134,17 +140,36 @@ int __get_instruction_length_from_list(s
while ( (inst_len < MAX_INST_LEN) &&
is_prefix(buf[inst_len]) &&
!is_prefix(opcode[1]) )
+ {
inst_len++;
+ if ( inst_len >= fetch_len )
+ {
+ if ( !fetch(v, buf + fetch_len,
+ fetch_addr + fetch_len,
+ MAX_INST_LEN - fetch_len) )
+ return 0;
+ fetch_len = MAX_INST_LEN;
+ }
+ }
ASSERT(opcode[0] <= 15); /* Make sure the table is correct. */
found = 1;
- for ( i = 0; i < opcode[0]; i++ )
+ for ( i = 0; i < opcode[0] && inst_len + i <
MAX_INST_LEN; i++ )
{
/* If the last byte is zero, we just accept it without checking */
if ( (i == (opcode[0]-1)) && (opcode[i+1] == 0) )
break;
+ if ( inst_len + i >= fetch_len )
+ {
+ if ( !fetch(v, buf + fetch_len,
+ fetch_addr + fetch_len,
+ MAX_INST_LEN - fetch_len) )
+ return 0;
+ fetch_len = MAX_INST_LEN;
+ }
+
if ( buf[inst_len+i] != opcode[i+1] )
{
found = 0;
@@ -158,13 +183,12 @@ int __get_instruction_length_from_list(s
printk("%s: Mismatch between expected and actual instruction bytes:
"
"eip = %lx\n", __func__, (unsigned long)vmcb->rip);
+ domain_crash(v->domain);
return 0;
done:
inst_len += opcode[0];
ASSERT(inst_len <= MAX_INST_LEN);
- if ( match )
- *match = instr;
return inst_len;
}
diff -r 483d006cc607 -r c20b8931b598 xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c Fri Apr 25 13:46:27 2008 +0100
+++ b/xen/arch/x86/hvm/svm/svm.c Fri May 02 16:28:05 2008 +0100
@@ -84,7 +84,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);
@@ -907,7 +910,7 @@ static void svm_vmexit_do_cpuid(struct c
{
unsigned int eax, ebx, ecx, edx, inst_len;
- inst_len = __get_instruction_length(current, INSTR_CPUID, NULL);
+ inst_len = __get_instruction_length(current, INSTR_CPUID);
if ( inst_len == 0 )
return;
@@ -1084,12 +1087,12 @@ static void svm_do_msr_access(struct cpu
if ( vmcb->exitinfo1 == 0 )
{
rc = hvm_msr_read_intercept(regs);
- inst_len = __get_instruction_length(v, INSTR_RDMSR, NULL);
+ inst_len = __get_instruction_length(v, INSTR_RDMSR);
}
else
{
rc = hvm_msr_write_intercept(regs);
- inst_len = __get_instruction_length(v, INSTR_WRMSR, NULL);
+ inst_len = __get_instruction_length(v, INSTR_WRMSR);
}
if ( rc == X86EMUL_OKAY )
@@ -1103,7 +1106,7 @@ static void svm_vmexit_do_hlt(struct vmc
struct hvm_intack intack = hvm_vcpu_has_pending_irq(curr);
unsigned int inst_len;
- inst_len = __get_instruction_length(curr, INSTR_HLT, NULL);
+ inst_len = __get_instruction_length(curr, INSTR_HLT);
if ( inst_len == 0 )
return;
__update_guest_eip(regs, inst_len);
@@ -1140,7 +1143,7 @@ static void svm_vmexit_do_invalidate_cac
svm_wbinvd_intercept();
inst_len = __get_instruction_length_from_list(
- current, list, ARRAY_SIZE(list), NULL, NULL);
+ current, list, ARRAY_SIZE(list));
__update_guest_eip(regs, inst_len);
}
@@ -1216,7 +1219,7 @@ asmlinkage void svm_vmexit_handler(struc
if ( !v->domain->debugger_attached )
goto exit_and_crash;
/* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
- inst_len = __get_instruction_length(v, INSTR_INT3, NULL);
+ inst_len = __get_instruction_length(v, INSTR_INT3);
__update_guest_eip(regs, inst_len);
domain_pause_for_debugger();
break;
@@ -1293,7 +1296,7 @@ asmlinkage void svm_vmexit_handler(struc
break;
case VMEXIT_VMMCALL:
- inst_len = __get_instruction_length(v, INSTR_VMCALL, NULL);
+ inst_len = __get_instruction_length(v, INSTR_VMCALL);
if ( inst_len == 0 )
break;
HVMTRACE_1D(VMMCALL, v, regs->eax);
diff -r 483d006cc607 -r c20b8931b598 xen/include/asm-x86/hvm/svm/emulate.h
--- a/xen/include/asm-x86/hvm/svm/emulate.h Fri Apr 25 13:46:27 2008 +0100
+++ b/xen/include/asm-x86/hvm/svm/emulate.h Fri May 02 16:28:05 2008 +0100
@@ -34,15 +34,12 @@ enum instruction_index {
};
int __get_instruction_length_from_list(
- struct vcpu *v,
- enum instruction_index *list, unsigned int list_count,
- u8 *guest_eip_buf, enum instruction_index *match);
+ struct vcpu *v, enum instruction_index *list, unsigned int list_count);
static inline int __get_instruction_length(struct vcpu *v,
- enum instruction_index instr, u8 *guest_eip_buf)
+ enum instruction_index instr)
{
- return __get_instruction_length_from_list(
- v, &instr, 1, guest_eip_buf, NULL);
+ return __get_instruction_length_from_list(v, &instr, 1);
}
#endif /* __ASM_X86_HVM_SVM_EMULATE_H__ */
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel