[PATCH stable 4.4] KVM: x86: Fix misplaced backport of "work around leak of uninitialized stack contents"

[PATCH stable 4.4] KVM: x86: Fix misplaced backport of "work around leak of uninitialized stack contents"

[Guillaume Bertholon]

The upstream commit 541ab2aeb282 ("KVM: x86: work around leak of
uninitialized stack contents") resets `exception` in the function
`kvm_write_guest_virt_system`.
However, its backported version in stable (commit ba7f1c934f2e
("KVM: x86: work around leak of uninitialized stack contents")) applied
the change in `emulator_write_std` instead.

This patch moves the memset instruction back to
`kvm_write_guest_virt_system`.

Fixes: ba7f1c934f2e ("KVM: x86: work around leak of uninitialized stack contents")
Signed-off-by: Guillaume Bertholon <guillaume.bertholon@ens.fr>
---
 arch/x86/kvm/x86.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8dce61c..9101002 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4417,13 +4417,6 @@ static int emulator_write_std(struct x86_emulate_ctxt *ctxt, gva_t addr, void *v
 	if (!system && kvm_x86_ops->get_cpl(vcpu) == 3)
 		access |= PFERR_USER_MASK;

-	/*
-	 * FIXME: this should call handle_emulation_failure if X86EMUL_IO_NEEDED
-	 * is returned, but our callers are not ready for that and they blindly
-	 * call kvm_inject_page_fault.  Ensure that they at least do not leak
-	 * uninitialized kernel stack memory into cr2 and error code.
-	 */
-	memset(exception, 0, sizeof(*exception));
 	return kvm_write_guest_virt_helper(addr, val, bytes, vcpu,
 					   access, exception);
 }
@@ -4431,6 +4424,13 @@ static int emulator_write_std(struct x86_emulate_ctxt *ctxt, gva_t addr, void *v
 int kvm_write_guest_virt_system(struct kvm_vcpu *vcpu, gva_t addr, void *val,
 				unsigned int bytes, struct x86_exception *exception)
 {
+	/*
+	 * FIXME: this should call handle_emulation_failure if X86EMUL_IO_NEEDED
+	 * is returned, but our callers are not ready for that and they blindly
+	 * call kvm_inject_page_fault.  Ensure that they at least do not leak
+	 * uninitialized kernel stack memory into cr2 and error code.
+	 */
+	memset(exception, 0, sizeof(*exception));
 	return kvm_write_guest_virt_helper(addr, val, bytes, vcpu,
 					   PFERR_WRITE_MASK, exception);
 }
--
2.7.4

Re: [PATCH stable 4.4] KVM: x86: Fix misplaced backport of "work around leak of uninitialized stack contents"

[Greg KH]

On Tue, Feb 01, 2022 at 06:17:51PM +0100, Guillaume Bertholon wrote:
> The upstream commit 541ab2aeb282 ("KVM: x86: work around leak of
> uninitialized stack contents") resets `exception` in the function
> `kvm_write_guest_virt_system`.
> However, its backported version in stable (commit ba7f1c934f2e
> ("KVM: x86: work around leak of uninitialized stack contents")) applied
> the change in `emulator_write_std` instead.
> 
> This patch moves the memset instruction back to
> `kvm_write_guest_virt_system`.
> 
> Fixes: ba7f1c934f2e ("KVM: x86: work around leak of uninitialized stack contents")
> Signed-off-by: Guillaume Bertholon <guillaume.bertholon@ens.fr>
> ---
>  arch/x86/kvm/x86.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8dce61c..9101002 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4417,13 +4417,6 @@ static int emulator_write_std(struct x86_emulate_ctxt *ctxt, gva_t addr, void *v
>  	if (!system && kvm_x86_ops->get_cpl(vcpu) == 3)
>  		access |= PFERR_USER_MASK;
> 
> -	/*
> -	 * FIXME: this should call handle_emulation_failure if X86EMUL_IO_NEEDED
> -	 * is returned, but our callers are not ready for that and they blindly
> -	 * call kvm_inject_page_fault.  Ensure that they at least do not leak
> -	 * uninitialized kernel stack memory into cr2 and error code.
> -	 */
> -	memset(exception, 0, sizeof(*exception));
>  	return kvm_write_guest_virt_helper(addr, val, bytes, vcpu,
>  					   access, exception);
>  }
> @@ -4431,6 +4424,13 @@ static int emulator_write_std(struct x86_emulate_ctxt *ctxt, gva_t addr, void *v
>  int kvm_write_guest_virt_system(struct kvm_vcpu *vcpu, gva_t addr, void *val,
>  				unsigned int bytes, struct x86_exception *exception)
>  {
> +	/*
> +	 * FIXME: this should call handle_emulation_failure if X86EMUL_IO_NEEDED
> +	 * is returned, but our callers are not ready for that and they blindly
> +	 * call kvm_inject_page_fault.  Ensure that they at least do not leak
> +	 * uninitialized kernel stack memory into cr2 and error code.
> +	 */
> +	memset(exception, 0, sizeof(*exception));
>  	return kvm_write_guest_virt_helper(addr, val, bytes, vcpu,
>  					   PFERR_WRITE_MASK, exception);
>  }
> --
> 2.7.4
> 

All 3 now queued up.

Note, 4.4.y is about to go end-of-life now, so I wouldn't spend much
more time on it if you do not want to.

thanks,

greg k-h

Re: [PATCH stable 4.4] KVM: x86: Fix misplaced backport of "work around leak of uninitialized stack contents"

[Guillaume Bertholon]

On Tue, 1 Feb 2022 18:52:04 +0100, Greg KH wrote:
> Note, 4.4.y is about to go end-of-life now, so I wouldn't spend much
> more time on it if you do not want to.

Since I already wrote it, I will send one last patch on the 4.4.y branch,
and then I will move to 4.9.y.

If you think it is a waste of your time, feel free to ignore it.

As a last contribution to 4.4, I can also report that backported
commit b7cf6750c05a ("drm/amdgpu: when suspending, if uvd/vce was running.
need to cancel delay work.") applies a fix for `amdgpu_uvd_suspend` in the
function `amdgpu_uvd_resume`, but I am not sure of where the instruction
should go, so I could not make a patch for this one.

Sorry for the bad timing,
- Guillaume Bertholon

Re: [PATCH stable 4.4] KVM: x86: Fix misplaced backport of "work around leak of uninitialized stack contents"

[Greg KH]

On Wed, Feb 02, 2022 at 03:00:28PM +0100, Guillaume Bertholon wrote:
> On Tue, 1 Feb 2022 18:52:04 +0100, Greg KH wrote:
> > Note, 4.4.y is about to go end-of-life now, so I wouldn't spend much
> > more time on it if you do not want to.
> 
> Since I already wrote it, I will send one last patch on the 4.4.y branch,
> and then I will move to 4.9.y.
> 
> If you think it is a waste of your time, feel free to ignore it.

I took it, thanks!

> As a last contribution to 4.4, I can also report that backported
> commit b7cf6750c05a ("drm/amdgpu: when suspending, if uvd/vce was running.
> need to cancel delay work.") applies a fix for `amdgpu_uvd_suspend` in the
> function `amdgpu_uvd_resume`, but I am not sure of where the instruction
> should go, so I could not make a patch for this one.

Ah, odds are no one using that hardware is still using the 4.4 kernel.
It probably should just be reverted, but I'll not worry about it now.

If you want to run your tests/scripts on the 4.9 or other stable kernel
branches, to verify we didn't do much the same type of mistake there,
that would be most appreciated.  This is good stuff, thanks for doing
this.

> Sorry for the bad timing,

Not at all, thank you for the fixes!

greg k-h