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@xxxxxxxxxxxxxxxxxxxxx>
> ---
> 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
>
