| Message ID | 20231004133827.107-1-julian.stecklina@cyberus-technology.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] KVM: x86: Fix partially uninitialized integer in emulate_pop | expand |
On Wed, Oct 04, 2023, 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. > > Fix this by explicitly requiring a unsigned long pointer and > initializing it with zero in all cases. NAK, this will break em_leave() as it will zero RBP regardless of how many bytes are actually supposed to be written. Specifically, KVM would incorrectly clobber RBP[31:16] if LEAVE is executed with a 16-bit stack. I generally like defense-in-depth approaches, but zeroing data that the caller did not ask to be written is not a net positive. > Signed-off-by: Julian Stecklina <julian.stecklina@cyberus-technology.de> > --- > arch/x86/kvm/emulate.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index 2673cd5c46cb..fc4a365a309f 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -1838,18 +1838,24 @@ static int em_push(struct x86_emulate_ctxt *ctxt) > } > > static int emulate_pop(struct x86_emulate_ctxt *ctxt, > - void *dest, int len) > + unsigned long *dest, u8 op_bytes) > { > int rc; > struct segmented_address addr; > > + /* > + * segmented_read below will only partially initialize dest when > + * we are not in 64-bit mode. > + */ > + *dest = 0; > + > addr.ea = reg_read(ctxt, VCPU_REGS_RSP) & stack_mask(ctxt); > addr.seg = VCPU_SREG_SS; > - rc = segmented_read(ctxt, addr, dest, len); > + rc = segmented_read(ctxt, addr, dest, op_bytes); > if (rc != X86EMUL_CONTINUE) > return rc; > > - rsp_increment(ctxt, len); > + rsp_increment(ctxt, op_bytes); > return rc; > } > > @@ -1999,7 +2005,7 @@ static int em_popa(struct x86_emulate_ctxt *ctxt) > { > int rc = X86EMUL_CONTINUE; > int reg = VCPU_REGS_RDI; > - u32 val; > + unsigned long val; > > while (reg >= VCPU_REGS_RAX) { > if (reg == VCPU_REGS_RSP) { > -- > 2.40.1 >
On Wed, 2023-10-04 at 08:07 -0700, Sean Christopherson wrote: > On Wed, Oct 04, 2023, 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. > > > > Fix this by explicitly requiring a unsigned long pointer and > > initializing it with zero in all cases. > > NAK, this will break em_leave() as it will zero RBP regardless of how many > bytes > are actually supposed to be written. Specifically, KVM would incorrectly > clobber > RBP[31:16] if LEAVE is executed with a 16-bit stack. Thanks, Sean! Great catch. I didn't see this. Is there already a test suite for this? > I generally like defense-in-depth approaches, but zeroing data that the caller > did not ask to be written is not a net positive. I'll rewrite the patch to just initialize variables where they are currently not. This should be a bit more conservative and have less risk of breaking anything. Julian
On Thu, Oct 05, 2023, Julian Stecklina wrote: > On Wed, 2023-10-04 at 08:07 -0700, Sean Christopherson wrote: > > On Wed, Oct 04, 2023, 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. > > > > > > Fix this by explicitly requiring a unsigned long pointer and > > > initializing it with zero in all cases. > > > > NAK, this will break em_leave() as it will zero RBP regardless of how many > > bytes > > are actually supposed to be written. Specifically, KVM would incorrectly > > clobber > > RBP[31:16] if LEAVE is executed with a 16-bit stack. > > Thanks, Sean! Great catch. I didn't see this. Is there already a test suite for > this? No, I'm just excessively paranoid when it comes to the emulator :-) > > I generally like defense-in-depth approaches, but zeroing data that the caller > > did not ask to be written is not a net positive. > > I'll rewrite the patch to just initialize variables where they are currently > not. This should be a bit more conservative and have less risk of breaking > anything. In all honesty, I wouldn't bother. Trying to harden the emulator code for things like this will be a never ending game of whack-a-mole. The operands, of which there are many, have multiple unions with fields of varying size, and all kinds of subtle rules/logic for which field is used, how many bytes within a given field are valid, etc. It pains me a bit to say this, but I think we're best off leaving the emulator as-is, and relying on things like fancy compiler features, UBSAN, and fuzzers to detect any lurking bugs. struct operand { enum { OP_REG, OP_MEM, OP_MEM_STR, OP_IMM, OP_XMM, OP_MM, OP_NONE } type; unsigned int bytes; unsigned int count; union { unsigned long orig_val; u64 orig_val64; }; union { unsigned long *reg; struct segmented_address { ulong ea; unsigned seg; } mem; unsigned xmm; unsigned mm; } addr; union { unsigned long val; u64 val64; char valptr[sizeof(sse128_t)]; sse128_t vec_val; u64 mm_val; void *data; }; };
On Fri, 2023-10-06 at 00:56 +0000, Sean Christopherson wrote: > On Thu, Oct 05, 2023, Julian Stecklina wrote: > > On Wed, 2023-10-04 at 08:07 -0700, Sean Christopherson wrote: > > > > > > NAK, this will break em_leave() as it will zero RBP regardless of how many > > > bytes > > > are actually supposed to be written. Specifically, KVM would incorrectly > > > clobber > > > RBP[31:16] if LEAVE is executed with a 16-bit stack. > > > > Thanks, Sean! Great catch. I didn't see this. Is there already a test suite > > for > > this? > > No, I'm just excessively paranoid when it comes to the emulator :-) I'll look into whether some testing can be added to kvm-unit-tests or maybe some other test harness. > It pains me a bit to say this, but I think we're best off leaving the emulator > as-is, and relying on things like fancy compiler features, UBSAN, and fuzzers > to > detect any lurking bugs. I'm have a fuzzing setup for the emulator in userspace. This issue was detected by MSAN. :) I'll make this available when it's in a better shape. So if you don't strongly mind , I would still initialize the places where the fuzzer can show that the code hands uninialized data around. At the least, it will make other fuzzing efforts a bit easier. But I do understand that changes need to be conservative. Btw, what are the cases where ret far, iret etc (basically anything you wouldn't expect for MMIO) are handled by the KVM emulator without the guest doing anything fishy? Julian
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 2673cd5c46cb..fc4a365a309f 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -1838,18 +1838,24 @@ static int em_push(struct x86_emulate_ctxt *ctxt) } static int emulate_pop(struct x86_emulate_ctxt *ctxt, - void *dest, int len) + unsigned long *dest, u8 op_bytes) { int rc; struct segmented_address addr; + /* + * segmented_read below will only partially initialize dest when + * we are not in 64-bit mode. + */ + *dest = 0; + addr.ea = reg_read(ctxt, VCPU_REGS_RSP) & stack_mask(ctxt); addr.seg = VCPU_SREG_SS; - rc = segmented_read(ctxt, addr, dest, len); + rc = segmented_read(ctxt, addr, dest, op_bytes); if (rc != X86EMUL_CONTINUE) return rc; - rsp_increment(ctxt, len); + rsp_increment(ctxt, op_bytes); return rc; } @@ -1999,7 +2005,7 @@ static int em_popa(struct x86_emulate_ctxt *ctxt) { int rc = X86EMUL_CONTINUE; int reg = VCPU_REGS_RDI; - u32 val; + unsigned long val; while (reg >= VCPU_REGS_RAX) { if (reg == VCPU_REGS_RSP) {
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. Fix this by explicitly requiring a unsigned long pointer and initializing it with zero in all cases. Signed-off-by: Julian Stecklina <julian.stecklina@cyberus-technology.de> --- arch/x86/kvm/emulate.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)