Re: [PATCH 1/3] KVM: x86: Fix KVM_GET_MSRS stack info leak

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 2/3/2024 8:45 PM, Mathias Krause wrote:
> Commit 6abe9c1386e5 ("KVM: X86: Move ignore_msrs handling upper the
> stack") changed the 'ignore_msrs' handling, including sanitizing return
> values to the caller. This was fine until commit 12bc2132b15e ("KVM:
> X86: Do the same ignore_msrs check for feature msrs") which allowed
> non-existing feature MSRs to be ignored, i.e. to not generate an error
> on the ioctl() level. It even tried to preserve the sanitization of the
> return value. However, the logic is flawed, as '*data' will be
> overwritten again with the uninitialized stack value of msr.data.
> 
> Fix this by simplifying the logic and always initializing msr.data,
> vanishing the need for an additional error exit path.
> 
> Fixes: 12bc2132b15e ("KVM: X86: Do the same ignore_msrs check for feature msrs")
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   arch/x86/kvm/x86.c | 15 +++++----------
>   1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 363b1c080205..13ec948f3241 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1704,22 +1704,17 @@ static int do_get_msr_feature(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
>   	struct kvm_msr_entry msr;
>   	int r;
>   
> +	/* Unconditionally clear the output for simplicity */
> +	msr.data = 0;
>   	msr.index = index;
>   	r = kvm_get_msr_feature(&msr);
>   
> -	if (r == KVM_MSR_RET_INVALID) {
> -		/* Unconditionally clear the output for simplicity */
> -		*data = 0;
> -		if (kvm_msr_ignored_check(index, 0, false))
> -			r = 0;
> -	}
> -
> -	if (r)
> -		return r;
> +	if (r == KVM_MSR_RET_INVALID && kvm_msr_ignored_check(index, 0, false))
> +		r = 0;
>   
>   	*data = msr.data;
>   
> -	return 0;
> +	return r;
>   }
>   
>   static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)