From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [patch RFC] kvm, cpuid: silence a buffer overflow warning Date: Thu, 20 Feb 2014 14:19:37 +0100 Message-ID: <530600E9.4010506@redhat.com> References: <20140220123419.GA10110@elgon.mountain> <5305FE1E.6060902@redhat.com> <5306009D.8040205@bfs.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Dan Carpenter , Gleb Natapov , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, kvm@vger.kernel.org, kernel-janitors@vger.kernel.org To: wharms@bfs.de Return-path: In-Reply-To: <5306009D.8040205@bfs.de> Sender: kernel-janitors-owner@vger.kernel.org List-Id: kvm.vger.kernel.org Il 20/02/2014 14:18, walter harms ha scritto: > > > Am 20.02.2014 14:07, schrieb Paolo Bonzini: >> 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? >> > > Is there any guaranty that this will not loop forever ? > > an if (i==j) return 0; would be on the save side. (I guess that > these was the idea behind the for). Once i==j you'll get ej->function == e->function and exit. Paolo