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

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

[Vitaly Kuznetsov]

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.

While on it, drop the unneeded 'consumed_xmm_halves' initializer. In
'hc->fast' case the variable is always initialized and is not used
otherwise, 'data_offset' is not being initialized either.

Reported-by: coverity-bot <keescook+coverity-bot@chromium.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@redhat.com>
---
 arch/x86/kvm/hyperv.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 2c7f2a26421e..dee4961ad8ff 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1926,7 +1926,7 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc)
 	struct kvm_vcpu *v;
 	unsigned long i;
 	bool all_cpus;
-	int consumed_xmm_halves = 0;
+	int consumed_xmm_halves;
 	gpa_t data_offset;
 
 	/*
@@ -2021,8 +2021,10 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc)
 		 * consumed_xmm_halves to make sure TLB flush entries are read
 		 * from the correct offset.
 		 */
-		data_offset += hc->var_cnt * sizeof(sparse_banks[0]);
-		consumed_xmm_halves += hc->var_cnt;
+		if (hc->fast)
+			consumed_xmm_halves += hc->var_cnt;
+		else
+			data_offset += hc->var_cnt * sizeof(sparse_banks[0]);
 	}
 
 	if (hc->code == HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE ||
-- 
2.38.1

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

[Sean Christopherson]

On Fri, Dec 02, 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.

If those calls aren't inlined, then 32-bit Hyper-V will be "consuming" uninitialized
data when pushing parameters onto the stack.  It won't cause real problems, but
checkers might complain.

What about shoving this metadata into "struct kvm_hv_hcall" as a union?  That'd
help convey that the two are mutually exclusive, would provide a place to document
said exclusion, and would yield a nice cleanup too by eliminating multiple params
from various functions.

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

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Fri, Dec 02, 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.
>
> If those calls aren't inlined, then 32-bit Hyper-V will be "consuming" uninitialized
> data when pushing parameters onto the stack.  It won't cause real problems, but
> checkers might complain.
>
> What about shoving this metadata into "struct kvm_hv_hcall" as a union?  That'd
> help convey that the two are mutually exclusive, would provide a place to document
> said exclusion, and would yield a nice cleanup too by eliminating multiple params
> from various functions.

"struct kvm_hv_hcall" used to hold raw data from the guest and
'consumed_xmm_halves'/ 'data_offset' are rather our implementation
details, how we consume these data. I don't see why we can't re-purpose
it a little bit to hold both, let me try that in v2.

-- 
Vitaly