Re: [PATCH 00/20] objtool: UACCESS validation v3

From: Josh Poimboeuf <jpoimboe@redhat.com>
To: hpa@zytor.com
Cc: Peter Zijlstra <peterz@infradead.org>,
	torvalds@linux-foundation.org, tglx@linutronix.de,
	julien.thierry@arm.com, will.deacon@arm.com, luto@amacapital.net,
	mingo@kernel.org, catalin.marinas@arm.com, james.morse@arm.com,
	valentin.schneider@arm.com, brgerst@gmail.com, luto@kernel.org,
	bp@alien8.de, dvlasenk@redhat.com, linux-kernel@vger.kernel.org,
	dvyukov@google.com, rostedt@goodmis.org
Subject: Re: [PATCH 00/20] objtool: UACCESS validation v3
Date: Thu, 7 Mar 2019 11:45:56 -0600	[thread overview]
Message-ID: <20190307174556.m7nh2y4wt3lbhuxd@treble> (raw)
In-Reply-To: <714E9D57-CCAE-4A69-AC3E-E6AFC525834E@zytor.com>

On Thu, Mar 07, 2019 at 09:29:17AM -0800, hpa@zytor.com wrote:
> On March 7, 2019 9:18:29 AM PST, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >On Thu, Mar 07, 2019 at 09:04:36AM -0800, hpa@zytor.com wrote:
> >> On March 7, 2019 8:47:05 AM PST, Josh Poimboeuf <jpoimboe@redhat.com>
> >wrote:
> >> >On Thu, Mar 07, 2019 at 02:13:12PM +0100, Peter Zijlstra wrote:
> >> >> On Thu, Mar 07, 2019 at 01:55:26PM +0100, Peter Zijlstra wrote:
> >> >> > On Thu, Mar 07, 2019 at 01:03:17PM +0100, Peter Zijlstra wrote:
> >> >> 
> >> >> 
> >> >> > > 01be     20d3:  31 c0                   xor    %eax,%eax
> >> >> > > 01c0     20d5:  4c 39 eb                cmp    %r13,%rbx
> >> >> > > 01c3     20d8:  77 08                   ja     20e2
> >> ><__do_sys_waitid+0x1cd>
> >> >> 
> >> >> randconfig-build/kernel/exit.o: warning: objtool:  
> >> >__do_sys_waitid()+0x1c3: (branch)
> >> >> 
> >> >> > > 01cd     20e2:	83 f0 01             	xor    $0x1,%eax
> >> >> > > 01d0     20e5:	48 89 c2             	mov    %rax,%rdx
> >> >> > > 01d3     20e8:	83 e2 01             	and    $0x1,%edx
> >> >> > > 01d6     20eb:	48 83 c2 02          	add    $0x2,%rdx
> >> >> > > 01da     20ef:	48 ff 04 d5 00 00 00 	incq   0x0(,%rdx,8)
> >> >> > > 01e1     20f6:	00 
> >> >> > > 01de 			20f3: R_X86_64_32S	_ftrace_branch+0x148
> >> >> > > 01e2     20f7:	84 c0                	test   %al,%al
> >> >> > > 01e4     20f9:	75 2d                	jne    2128
> >> ><__do_sys_waitid+0x213>
> >> >> > 
> >> >> > we do not take this branch and fall-through.
> >> >> 
> >> >> And that is the error, I think. We should've taken it and went to:
> >> >> 
> >> >>   return -EFAULT;
> >> >> 
> >> >> because:
> >> >> 
> >> >>  +1be	xor  %eax,%eax	eax=0
> >> >>  +1cd   xor  $0x1,%eax	eax=1
> >> >>  +1e2   test %al,%al	1&1==1 -> ZF=0
> >> >>  +1e4   jnz
> >> >> 
> >> >> Is an unconditional code sequence, but there's no way objtool can
> >do
> >> >> that without becoming a full blown interpreter :/
> >> >> 
> >> >> > > 0213     2128:  49 c7 c7 f2 ff ff ff    mov   
> >> >$0xfffffffffffffff2,%r15
> >> >> > > ffffffffffffe0eb }
> >> >> > > 021a     212f:  48 8d 65 d8             lea   
> >-0x28(%rbp),%rsp
> >> >> > > 021e     2133:  4c 89 f8                mov    %r15,%rax
> >> >> > > 0221     2136:  5b                      pop    %rbx
> >> >> > > 0222     2137:  41 5c                   pop    %r12
> >> >> > > 0224     2139:  41 5d                   pop    %r13
> >> >> > > 0226     213b:  41 5e                   pop    %r14
> >> >> > > 0228     213d:  41 5f                   pop    %r15
> >> >> > > 022a     213f:  5d                      pop    %rbp
> >> >> > > 022b     2140:  c3                      retq
> >> >
> >> >This "fixes" it, and also seems to help -Os make much code:
> >> >
> >> >diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> >> >index 445348facea9..8de63db58fdd 100644
> >> >--- a/include/linux/compiler.h
> >> >+++ b/include/linux/compiler.h
> >> >@@ -67,7 +67,7 @@ void ftrace_likely_update(struct
> >ftrace_likely_data
> >> >*f, int val,
> >> > 				.line = __LINE__,			\
> >> > 			};						\
> >> > 		______r = !!(cond);					\
> >> >-		______f.miss_hit[______r]++;					\
> >> >+		if (______r) ______f.miss_hit[1]++; else ______f.miss_hit[0]++; \
> >> > 		______r;						\
> >> > 	}))
> >> > #endif /* CONFIG_PROFILE_ALL_BRANCHES */
> >> 
> >> if (cond)?  Or is ___r used elsewhere?
> >
> >______r is also the return value.  And it's needed because cond should
> >only be evaluated once.
> 
> So put a true; and false; inside the if.

Is that possible to do in a C macro?  Doesn't seem to work for me...

-- 
Josh