From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Date: Thu, 20 Feb 2014 13:07:42 +0000 Subject: Re: [patch RFC] kvm, cpuid: silence a buffer overflow warning Message-Id: <5305FE1E.6060902@redhat.com> List-Id: References: <20140220123419.GA10110@elgon.mountain> In-Reply-To: <20140220123419.GA10110@elgon.mountain> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Dan Carpenter , Gleb Natapov Cc: Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, kvm@vger.kernel.org, kernel-janitors@vger.kernel.org Il 20/02/2014 13:34, Dan Carpenter ha scritto: > This seems like a harmless off by one overflow if "i" is the last > element in the vcpu->arch.cpuid_entries[] array. > > Signed-off-by: Dan Carpenter > --- > Not tested. I always wonder if it's worth fixing these or if it's worth > reporting them? Either of those seem like a lot of work for something > harmless. Could it oops if cpuid_nent is INT_MAX? If so, it's not entirely harmless. In this case I'd rather take the occasion to cleanup the code like this (compile-tested): diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 9fed5bedaad6..2fd6e7169936 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -656,18 +656,19 @@ out: static int move_to_next_stateful_cpuid_entry(struct kvm_vcpu *vcpu, int i) { struct kvm_cpuid_entry2 *e = &vcpu->arch.cpuid_entries[i]; - int j, nent = vcpu->arch.cpuid_nent; + struct kvm_cpuid_entry2 *ej; + int j = i, nent = vcpu->arch.cpuid_nent; e->flags &= ~KVM_CPUID_FLAG_STATE_READ_NEXT; + /* when no next entry is found, the current entry[i] is reselected */ - for (j = i + 1; ; j = (j + 1) % nent) { - struct kvm_cpuid_entry2 *ej = &vcpu->arch.cpuid_entries[j]; - if (ej->function = e->function) { - ej->flags |= KVM_CPUID_FLAG_STATE_READ_NEXT; - return j; - } - } - return 0; /* silence gcc, even though control never reaches here */ + do { + j = (j + 1) % nent; + ej = &vcpu->arch.cpuid_entries[j]; + } while (ej->function != e->function); + + ej->flags |= KVM_CPUID_FLAG_STATE_READ_NEXT; + return j; } /* find an entry with matching function, matching index (if needed), and that What do you think? Paolo