linux-kernel - Re: [GIT PULL] Early batch of KVM changes for 4.3 merge window

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]

Date:	Tue, 1 Sep 2015 19:03:56 +0200
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Xiao Guangrong <guangrong.xiao@...ux.intel.com>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Gleb Natapov <gleb@...nel.org>, KVM list <kvm@...r.kernel.org>
Subject: Re: [GIT PULL] Early batch of KVM changes for 4.3 merge window



On 01/09/2015 02:47, Linus Torvalds wrote:
> Hmm:
> 
> On Fri, Aug 14, 2015 at 4:57 PM, Paolo Bonzini <pbonzini@...hat.com> wrote:
>>
>> Xiao Guangrong (9):
>>       KVM: MMU: fully check zero bits for sptes
> 
> The above commit causes an annoying new compiler warning.
> 
> The warning is bogus ("variable 'leaf' possibly uninitialized"),
> because the use of the variable is protected by the 'bool reserved'
> flag, but gcc is apparently not smart enough to understand that.

Unfortunately it doesn't reproduce on all compiler versions.

Something like this should do it:

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index fb16a8ea3dee..3c745f3abde8 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3309,13 +3309,13 @@ walk_shadow_page_get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
 
 	walk_shadow_page_lockless_begin(vcpu);
 
-	for (shadow_walk_init(&iterator, vcpu, addr), root = iterator.level;
+	for (shadow_walk_init(&iterator, vcpu, addr),
+		 leaf = root = iterator.level;
 	     shadow_walk_okay(&iterator);
 	     __shadow_walk_next(&iterator, spte)) {
-		leaf = iterator.level;
 		spte = mmu_spte_get_lockless(iterator.sptep);
 
-		sptes[leaf - 1] = spte;
+		sptes[--leaf] = spte;
 
 		if (!is_shadow_present_pte(spte))
 			break;
@@ -3329,7 +3329,7 @@ walk_shadow_page_get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
 	if (reserved) {
 		pr_err("%s: detect reserved bits on spte, addr 0x%llx, dump hierarchy:\n",
 		       __func__, addr);
-		while (root >= leaf) {
+		while (root > leaf) {
 			pr_err("------ spte 0x%llx level %d.\n",
 			       sptes[root - 1], root);
 			root--;


But honestly I haven't even compiled it yet.  Xiao, what do you think?

Paolo

> Since bogus warnings cause people to possibly ignore the *real*
> warnings, this should be fixed. Maybe the code should get rid of that
> 'reserved' flag, and instead initialize "leaf" to zero, and use that
> as the flag instead (since zero isn't a valid level)? That would
> actually avoid an extra variable, and would get rid of the warning.
> 
> Hmm?
> 
>                  Linus
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/