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 <[email protected]>
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 <[email protected]>
---
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
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.
Sean Christopherson <[email protected]> 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