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 <[email protected]>
---
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
push and emulate_pop are counterparts. Rename push to emulate_push and
harmonize its function signature with emulate_pop. This should remove
a bit of cognitive load when reading this code.
Signed-off-by: Julian Stecklina <[email protected]>
---
arch/x86/kvm/emulate.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index fc4a365a309f..33f3327ddfa7 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1819,22 +1819,23 @@ static int writeback(struct x86_emulate_ctxt *ctxt, struct operand *op)
return X86EMUL_CONTINUE;
}
-static int push(struct x86_emulate_ctxt *ctxt, void *data, int bytes)
+static int emulate_push(struct x86_emulate_ctxt *ctxt, const unsigned long *data,
+ u8 op_bytes)
{
struct segmented_address addr;
- rsp_increment(ctxt, -bytes);
+ rsp_increment(ctxt, -(int)op_bytes);
addr.ea = reg_read(ctxt, VCPU_REGS_RSP) & stack_mask(ctxt);
addr.seg = VCPU_SREG_SS;
- return segmented_write(ctxt, addr, data, bytes);
+ return segmented_write(ctxt, addr, data, op_bytes);
}
static int em_push(struct x86_emulate_ctxt *ctxt)
{
/* Disable writeback. */
ctxt->dst.type = OP_NONE;
- return push(ctxt, &ctxt->src.val, ctxt->op_bytes);
+ return emulate_push(ctxt, &ctxt->src.val, ctxt->op_bytes);
}
static int emulate_pop(struct x86_emulate_ctxt *ctxt,
@@ -1925,7 +1926,7 @@ static int em_enter(struct x86_emulate_ctxt *ctxt)
return X86EMUL_UNHANDLEABLE;
rbp = reg_read(ctxt, VCPU_REGS_RBP);
- rc = push(ctxt, &rbp, stack_size(ctxt));
+ rc = emulate_push(ctxt, &rbp, stack_size(ctxt));
if (rc != X86EMUL_CONTINUE)
return rc;
assign_masked(reg_rmw(ctxt, VCPU_REGS_RBP), reg_read(ctxt, VCPU_REGS_RSP),
--
2.40.1
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 <[email protected]>
> ---
> 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, Oct 04, 2023, Julian Stecklina wrote:
> push and emulate_pop are counterparts. Rename push to emulate_push and
> harmonize its function signature with emulate_pop. This should remove
> a bit of cognitive load when reading this code.
>
> Signed-off-by: Julian Stecklina <[email protected]>
> ---
> arch/x86/kvm/emulate.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index fc4a365a309f..33f3327ddfa7 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -1819,22 +1819,23 @@ static int writeback(struct x86_emulate_ctxt *ctxt, struct operand *op)
> return X86EMUL_CONTINUE;
> }
>
> -static int push(struct x86_emulate_ctxt *ctxt, void *data, int bytes)
> +static int emulate_push(struct x86_emulate_ctxt *ctxt, const unsigned long *data,
> + u8 op_bytes)
I like the rename and making @data const, but please leave @bytes as an int.
Regarding @bytes versus @len, my vote is to do s/len/bytes for emulate_pop() and
emulate_popf().
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
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 <[email protected]>
---
arch/x86/kvm/emulate.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
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)
--
2.40.1
push and emulate_pop are counterparts. Rename push to emulate_push and
harmonize its function signature with emulate_pop. This should remove
a bit of cognitive load when reading this code.
Signed-off-by: Julian Stecklina <[email protected]>
---
arch/x86/kvm/emulate.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 86d0ee9f1a6a..1b42d85694c2 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1819,22 +1819,23 @@ static int writeback(struct x86_emulate_ctxt *ctxt, struct operand *op)
return X86EMUL_CONTINUE;
}
-static int push(struct x86_emulate_ctxt *ctxt, void *data, int bytes)
+static int emulate_push(struct x86_emulate_ctxt *ctxt, const void *data,
+ int len)
{
struct segmented_address addr;
- rsp_increment(ctxt, -bytes);
+ rsp_increment(ctxt, -len);
addr.ea = reg_read(ctxt, VCPU_REGS_RSP) & stack_mask(ctxt);
addr.seg = VCPU_SREG_SS;
- return segmented_write(ctxt, addr, data, bytes);
+ return segmented_write(ctxt, addr, data, len);
}
static int em_push(struct x86_emulate_ctxt *ctxt)
{
/* Disable writeback. */
ctxt->dst.type = OP_NONE;
- return push(ctxt, &ctxt->src.val, ctxt->op_bytes);
+ return emulate_push(ctxt, &ctxt->src.val, ctxt->op_bytes);
}
static int emulate_pop(struct x86_emulate_ctxt *ctxt,
@@ -1920,7 +1921,7 @@ static int em_enter(struct x86_emulate_ctxt *ctxt)
return X86EMUL_UNHANDLEABLE;
rbp = reg_read(ctxt, VCPU_REGS_RBP);
- rc = push(ctxt, &rbp, stack_size(ctxt));
+ rc = emulate_push(ctxt, &rbp, stack_size(ctxt));
if (rc != X86EMUL_CONTINUE)
return rc;
assign_masked(reg_rmw(ctxt, VCPU_REGS_RBP), reg_read(ctxt, VCPU_REGS_RSP),
--
2.40.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