Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Wed, 4 Oct 2023 08:07:02 -0700
In-Reply-To: <20231004133827.107-1-julian.stecklina@cyberus-technology.de>
Mime-Version: 1.0
References: <20231004133827.107-1-julian.stecklina@cyberus-technology.de>
Message-ID: <ZR1_lizQd14pbXbg@google.com>
Subject: Re: [PATCH 1/2] KVM: x86: Fix partially uninitialized integer in emulate_pop
From:   Sean Christopherson <seanjc@google.com>
To:     Julian Stecklina <julian.stecklina@cyberus-technology.de>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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
>