linux-kernel - Re: [PATCH 1/2] KVM: x86: Fix partially uninitialized integer in emulate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZR1_lizQd14pbXbg@google.com>
Date:   Wed, 4 Oct 2023 08:07:02 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Julian Stecklina <julian.stecklina@...erus-technology.de>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] KVM: x86: Fix partially uninitialized integer in emulate_pop

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@...erus-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
>