linux-kernel - Re: [PATCH v2] KVM: x86: hyper-v: Fix 'using uninitialized value' Coverity warning

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: <Y5KLfZj5+4W5ZlcT@google.com>
Date:   Fri, 9 Dec 2022 01:12:29 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        coverity-bot <keescook@...omium.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] KVM: x86: hyper-v: Fix 'using uninitialized value'
 Coverity warning

On Thu, Dec 08, 2022, Vitaly Kuznetsov wrote:
> In kvm_hv_flush_tlb(), 'data_offset' and 'consumed_xmm_halves' variables
> are used in a mutually exclusive way: in 'hc->fast' we count in 'XMM
> halves' and increase 'data_offset' otherwise. Coverity discovered, that in
> one case both variables are incremented unconditionally. This doesn't seem
> to cause any issues as the only user of 'data_offset'/'consumed_xmm_halves'
> data is kvm_hv_get_tlb_flush_entries() -> kvm_hv_get_hc_data() which also
> takes into account 'hc->fast' but is still worth fixing.
> 
> To make things explicit, put 'data_offset' and 'consumed_xmm_halves' to
> 'struct kvm_hv_hcall' as a union and use at call sites. This allows to
> remove explicit 'data_offset'/'consumed_xmm_halves' parameters from
> kvm_hv_get_hc_data()/kvm_get_sparse_vp_set()/kvm_hv_get_tlb_flush_entries()
> helpers.
> 
> Note: 'struct kvm_hv_hcall' is allocated on stack in kvm_hv_hypercall() and
> is not zeroed, consumers are supposed to initialize the appropriate field
> if needed.
> 
> Reported-by: coverity-bot <keescook+coverity-bot@...omium.org>
> Addresses-Coverity-ID: 1527764 ("Uninitialized variables")
> Fixes: 260970862c88 ("KVM: x86: hyper-v: Handle HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST{,EX} calls gently")
> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@...gle.com>

> Changes since v1:
> - Shove 'data_offset'/'consumed_xmm_halves' into a union in 'struct
> kvm_hv_hcall' to make things more explicit. [Sean]
> ---
>  arch/x86/kvm/hyperv.c | 63 ++++++++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 2c7f2a26421e..e8296942a868 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1769,6 +1769,7 @@ static bool hv_is_vp_in_sparse_set(u32 vp_id, u64 valid_bank_mask, u64 sparse_ba
>  }
>  
>  struct kvm_hv_hcall {
> +	/* Hypercall input data */
>  	u64 param;
>  	u64 ingpa;
>  	u64 outgpa;
> @@ -1779,12 +1780,21 @@ struct kvm_hv_hcall {
>  	bool fast;
>  	bool rep;
>  	sse128_t xmm[HV_HYPERCALL_MAX_XMM_REGISTERS];

Uber nit, might want an

	/* End hypercall input data */

or so to cleary demarcate the input vs. the scratch area.