linux-kernel - Re: Silence compiler warning in arch/x86/kvm/emulate.c

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:	Fri, 19 Feb 2016 13:05:10 +0100
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	Valdis Kletnieks <Valdis.Kletnieks@...edu>,
	Gleb Natapov <gleb@...nel.org>, x86@...nel.org,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	Aruna Hewapathirane <aruna.hewapathirane@...il.com>
Subject: Re: Silence compiler warning in arch/x86/kvm/emulate.c



On 19/02/2016 12:11, Aurelien Jarno wrote:
> On 2015-08-29 17:49, Valdis Kletnieks wrote:
>> Compiler warning:
>>
>>  CC [M]  arch/x86/kvm/emulate.o
>> arch/x86/kvm/emulate.c: In function "__do_insn_fetch_bytes":
>> arch/x86/kvm/emulate.c:814:9: warning: "linear" may be used uninitialized in this function [-Wmaybe-uninitialized]
>>
>> GCC is smart enough to realize that the inlined __linearize may return before
>> setting the value of linear, but not smart enough to realize the same
>> X86EMU_CONTINUE blocks actual use of the value.  However, the value of
>> 'linear' can only be set to one value, so hoisting the one line of code
>> upwards makes GCC happy with the code.
>>
>> Reported-by: Aruna Hewapathirane <aruna.hewapathirane@...il.com>
>> Tested-by: Aruna Hewapathirane <aruna.hewapathirane@...il.com>
>> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@...edu>
>>
>> --- a/arch/x86/kvm/emulate.c.dist	2015-08-11 14:10:05.366061993 -0400
>> +++ b/arch/x86/kvm/emulate.c	2015-08-29 13:43:13.014163958 -0400
>> @@ -650,6 +650,7 @@ static __always_inline int __linearize(s
>>  	u16 sel;
>>  
>>  	la = seg_base(ctxt, addr.seg) + addr.ea;
>> +	*linear = la;
>>  	*max_size = 0;
>>  	switch (mode) {
>>  	case X86EMUL_MODE_PROT64:
>> @@ -693,7 +694,6 @@ static __always_inline int __linearize(s
>>  	}
>>  	if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0))
>>  		return emulate_gp(ctxt, 0);
>> -	*linear = la;
>>  	return X86EMUL_CONTINUE;
>>  bad:
>>  	if (addr.seg == VCPU_SREG_SS)
>>
> 
> Unfortunately this patch broke GNU/Hurd when running under KVM. It fails
> to boot almost immediately. I haven't debug it more, but it looks like
> *linear should not always be written. This can easily be reproduced by
> trying to boot Debian Installer from this ISO:
> 
> http://ftp.debian-ports.org/debian-cd/hurd-i386/debian-hurd-2015/debian-hurd-2015-i386-CD-1.iso

The bug is that la can be changed by the "la &= (u32)-1" line.

So the fix could be like:

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 1505587d06e9..b9b09fec173b 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -650,10 +650,10 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
 	u16 sel;
 
 	la = seg_base(ctxt, addr.seg) + addr.ea;
-	*linear = la;
 	*max_size = 0;
 	switch (mode) {
 	case X86EMUL_MODE_PROT64:
+		*linear = la;
 		if (is_noncanonical_address(la))
 			goto bad;
 
@@ -662,6 +662,7 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
 			goto bad;
 		break;
 	default:
+		*linear = la = (u32)la;
 		usable = ctxt->ops->get_segment(ctxt, &sel, &desc, NULL,
 						addr.seg);
 		if (!usable)
@@ -689,7 +690,6 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
 			if (size > *max_size)
 				goto bad;
 		}
-		la &= (u32)-1;
 		break;
 	}
 	if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0))


Can you test it?

Paolo