x86/svm: Don't clobber eax and edx if an RDMSR intercept fails

The original code has a bug; eax and edx get unconditionally updated even when
hvm_msr_read_intercept() doesn't return X86EMUL_OKAY.

It is only by blind luck (vmce_rdmsr() eagerly initialising its msr_content
pointer) that this isn't an information leak into guests.

While fixing this bug, reduce the scope of msr_content and initialise it to 0.
This makes it obvious that a stack leak won't occur, even if there were to be
a buggy codepath in hvm_msr_read_intercept().

Also make some non-functional improvements.  Make the insn_len calculation
common, and reduce the quantity of explicit casting by making better use of
the existing register names.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Release-acked-by: Wei Liu <wei.liu2@citrix.com>

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 16427f62bbe72101660be9d3edfd35bb47f625c6..c9dbbea42fc14f424de824c2f4bccab100317442 100644 (file)
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1948,26 +1948,28 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
 
 static void svm_do_msr_access(struct cpu_user_regs *regs)
 {
-    int rc, inst_len;
-    struct vcpu *v = current;
-    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
-    uint64_t msr_content;
+    struct vcpu *curr = current;
+    bool rdmsr = curr->arch.hvm_svm.vmcb->exitinfo1 == 0;
+    int rc, inst_len = __get_instruction_length(
+        curr, rdmsr ? INSTR_RDMSR : INSTR_WRMSR);
+
+    if ( inst_len == 0 )
+        return;
 
-    if ( vmcb->exitinfo1 == 0 )
+    if ( rdmsr )
     {
-        if ( (inst_len = __get_instruction_length(v, INSTR_RDMSR)) == 0 )
-            return;
-        rc = hvm_msr_read_intercept(regs->ecx, &msr_content);
-        regs->eax = (uint32_t)msr_content;
-        regs->edx = (uint32_t)(msr_content >> 32);
+        uint64_t msr_content = 0;
+
+        rc = hvm_msr_read_intercept(regs->_ecx, &msr_content);
+        if ( rc == X86EMUL_OKAY )
+        {
+            regs->rax = (uint32_t)msr_content;
+            regs->rdx = (uint32_t)(msr_content >> 32);
+        }
     }
     else
-    {
-        if ( (inst_len = __get_instruction_length(v, INSTR_WRMSR)) == 0 )
-            return;
-        msr_content = ((uint64_t)regs->edx << 32) | (uint32_t)regs->eax;
-        rc = hvm_msr_write_intercept(regs->ecx, msr_content, 1);
-    }
+        rc = hvm_msr_write_intercept(regs->_ecx,
+                                     (regs->rdx << 32) | regs->_eax, 1);
 
     if ( rc == X86EMUL_OKAY )
         __update_guest_eip(regs, inst_len);