linux-kernel - Re: [PATCH v2 09/14] objtool: Add INSN_RETURN

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]

Message-ID: <20230622114504.GK4253@hirez.programming.kicks-ass.net>
Date:   Thu, 22 Jun 2023 13:45:04 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
Cc:     Michael Ellerman <mpe@...erman.id.au>,
        Nicholas Piggin <npiggin@...il.com>,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Sathvika Vasireddy <sv@...ux.ibm.com>,
        Naveen N Rao <naveen@...nel.org>,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v2 09/14] objtool: Add INSN_RETURN_CONDITIONAL

On Thu, Jun 22, 2023 at 12:54:31PM +0200, Christophe Leroy wrote:
> Most functions have an unconditional return at the end, like
> this one:
> 
> 	00000000 <is_exec_fault>:
> 	   0:	81 22 04 d0 	lwz     r9,1232(r2)
> 	   4:	38 60 00 00 	li      r3,0
> 	   8:	2c 09 00 00 	cmpwi   r9,0
> 	   c:	4d 82 00 20 	beqlr		<== Conditional return
> 	  10:	80 69 00 a0 	lwz     r3,160(r9)
> 	  14:	54 63 00 36 	clrrwi  r3,r3,4
> 	  18:	68 63 04 00 	xori    r3,r3,1024
> 	  1c:	7c 63 00 34 	cntlzw  r3,r3
> 	  20:	54 63 d9 7e 	srwi    r3,r3,5
> 	  24:	4e 80 00 20 	blr		<== Unconditional return
> 
> But other functions like this other one below only have
> conditional returns:
> 
> 	00000028 <pte_update.isra.0>:
> 	  28:	81 25 00 00 	lwz     r9,0(r5)
> 	  2c:	2c 08 00 00 	cmpwi   r8,0
> 	  30:	7d 29 30 78 	andc    r9,r9,r6
> 	  34:	7d 27 3b 78 	or      r7,r9,r7
> 	  38:	54 84 65 3a 	rlwinm  r4,r4,12,20,29
> 	  3c:	81 23 00 18 	lwz     r9,24(r3)
> 	  40:	41 82 00 58 	beq     98 <pte_update.isra.0+0x70>
> 	  44:	7d 29 20 2e 	lwzx    r9,r9,r4
> 	  48:	55 29 07 3a 	rlwinm  r9,r9,0,28,29
> 	  4c:	2c 09 00 0c 	cmpwi   r9,12
> 	  50:	41 82 00 08 	beq     58 <pte_update.isra.0+0x30>
> 	  54:	39 00 00 80 	li      r8,128
> 	  58:	2c 08 00 01 	cmpwi   r8,1
> 	  5c:	90 e5 00 00 	stw     r7,0(r5)
> 	  60:	4d a2 00 20 	beqlr+		<== Conditional return
> 	  64:	7c e9 3b 78 	mr      r9,r7
> 	  68:	39 40 00 00 	li      r10,0
> 	  6c:	39 4a 00 04 	addi    r10,r10,4
> 	  70:	7c 0a 40 00 	cmpw    r10,r8
> 	  74:	91 25 00 04 	stw     r9,4(r5)
> 	  78:	91 25 00 08 	stw     r9,8(r5)
> 	  7c:	38 a5 00 10 	addi    r5,r5,16
> 	  80:	91 25 ff fc 	stw     r9,-4(r5)
> 	  84:	4c 80 00 20 	bgelr		<== Conditional return
> 	  88:	55 49 60 26 	slwi    r9,r10,12
> 	  8c:	7d 29 3a 14 	add     r9,r9,r7
> 	  90:	91 25 00 00 	stw     r9,0(r5)
> 	  94:	4b ff ff d8 	b       6c <pte_update.isra.0+0x44>
> 	  98:	39 00 00 04 	li      r8,4
> 	  9c:	4b ff ff bc 	b       58 <pte_update.isra.0+0x30>
> 
> If conditional returns are decoded as INSN_OTHER, objtool considers
> that the second function never returns.
> 
> If conditional returns are decoded as INSN_RETURN, objtool considers
> that code after that conditional return is dead.
> 
> To overcome this situation, introduce INSN_RETURN_CONDITIONAL which
> is taken as a confirmation that a function is not noreturn but still
> sees following code as reachable.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>

Acked-by: Peter Zijlstra (Intel) <peterz@...radead.org>

> ---
>  tools/objtool/check.c                | 2 +-
>  tools/objtool/include/objtool/arch.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 0fcf99c91400..8977cdf93f54 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -259,7 +259,7 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
>  	func_for_each_insn(file, func, insn) {
>  		empty = false;
>  
> -		if (insn->type == INSN_RETURN)
> +		if (insn->type == INSN_RETURN || insn->type == INSN_RETURN_CONDITIONAL)
>  			return false;
>  	}
>  
> diff --git a/tools/objtool/include/objtool/arch.h b/tools/objtool/include/objtool/arch.h
> index 2b6d2ce4f9a5..84ba75112934 100644
> --- a/tools/objtool/include/objtool/arch.h
> +++ b/tools/objtool/include/objtool/arch.h
> @@ -19,6 +19,7 @@ enum insn_type {
>  	INSN_CALL,
>  	INSN_CALL_DYNAMIC,
>  	INSN_RETURN,
> +	INSN_RETURN_CONDITIONAL,
>  	INSN_CONTEXT_SWITCH,
>  	INSN_BUG,
>  	INSN_NOP,
> -- 
> 2.40.1
>