[v2,1/2] KVM: x86: Fix partially uninitialized integer in emulate_pop

Commit Message

Julian Stecklina Oct. 9, 2023, 9:20 a.m. UTC

Most code gives a pointer to an uninitialized unsigned long as dest in
emulate_pop. len is usually the word width of the guest.

If the guest runs in 16-bit or 32-bit modes, len will not cover the
whole unsigned long and we end up with uninitialized data in dest.

Looking through the callers of this function, the issue seems
harmless, but given that none of this is performance critical, there
should be no issue with just always initializing the whole value.

Even though popa is not reachable in 64-bit mode, I've changed its val
variable to unsigned long too to avoid sad copy'n'paste bugs.

Signed-off-by: Julian Stecklina <[e-mail]>
---
 arch/x86/kvm/emulate.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Sean Christopherson Feb. 9, 2024, 12:22 a.m. UTC | #1

On Mon, 09 Oct 2023 11:20:53 +0200, Julian Stecklina wrote:
> Most code gives a pointer to an uninitialized unsigned long as dest in
> emulate_pop. len is usually the word width of the guest.
> 
> If the guest runs in 16-bit or 32-bit modes, len will not cover the
> whole unsigned long and we end up with uninitialized data in dest.
> 
> Looking through the callers of this function, the issue seems
> harmless, but given that none of this is performance critical, there
> should be no issue with just always initializing the whole value.
> 
> [...]

Applied to kvm-x86 misc.  I massaged the changelog to make it clear that
uninitialized tweaks aren't actually a fix.  I also omitted the change from
a u32=>unsigned long.  The odds of someone copy+pasting em_popa() are lower
than the odds of an unnecessary size change causing some goofy error.

[1/2] KVM: x86: Clean up partially uninitialized integer in emulate_pop()
      https://github.com/kvm-x86/linux/commit/6fd1e3963f20
[2/2] KVM: x86: rename push to emulate_push for consistency
      https://github.com/kvm-x86/linux/commit/64435aaa4a6a

--
https://github.com/kvm-x86/linux/tree/next

Patch

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 2673cd5c46cb..86d0ee9f1a6a 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1862,7 +1862,8 @@  static int emulate_popf(struct x86_emulate_ctxt *ctxt,
 			void *dest, int len)
 {
 	int rc;
-	unsigned long val, change_mask;
+	unsigned long val = 0;
+	unsigned long change_mask;
 	int iopl = (ctxt->eflags & X86_EFLAGS_IOPL) >> X86_EFLAGS_IOPL_BIT;
 	int cpl = ctxt->ops->cpl(ctxt);
 
@@ -1953,7 +1954,7 @@  static int em_push_sreg(struct x86_emulate_ctxt *ctxt)
 static int em_pop_sreg(struct x86_emulate_ctxt *ctxt)
 {
 	int seg = ctxt->src2.val;
-	unsigned long selector;
+	unsigned long selector = 0;
 	int rc;
 
 	rc = emulate_pop(ctxt, &selector, 2);
@@ -1999,7 +2000,7 @@  static int em_popa(struct x86_emulate_ctxt *ctxt)
 {
 	int rc = X86EMUL_CONTINUE;
 	int reg = VCPU_REGS_RDI;
-	u32 val;
+	unsigned long val = 0;
 
 	while (reg >= VCPU_REGS_RAX) {
 		if (reg == VCPU_REGS_RSP) {
@@ -2228,7 +2229,7 @@  static int em_cmpxchg8b(struct x86_emulate_ctxt *ctxt)
 static int em_ret(struct x86_emulate_ctxt *ctxt)
 {
 	int rc;
-	unsigned long eip;
+	unsigned long eip = 0;
 
 	rc = emulate_pop(ctxt, &eip, ctxt->op_bytes);
 	if (rc != X86EMUL_CONTINUE)
@@ -2240,7 +2241,8 @@  static int em_ret(struct x86_emulate_ctxt *ctxt)
 static int em_ret_far(struct x86_emulate_ctxt *ctxt)
 {
 	int rc;
-	unsigned long eip, cs;
+	unsigned long eip = 0;
+	unsigned long cs = 0;
 	int cpl = ctxt->ops->cpl(ctxt);
 	struct desc_struct new_desc;
 
@@ -3183,7 +3185,7 @@  static int em_call_far(struct x86_emulate_ctxt *ctxt)
 static int em_ret_near_imm(struct x86_emulate_ctxt *ctxt)
 {
 	int rc;
-	unsigned long eip;
+	unsigned long eip = 0;
 
 	rc = emulate_pop(ctxt, &eip, ctxt->op_bytes);
 	if (rc != X86EMUL_CONTINUE)