linux-kernel - Re: [PATCH v5 02/17] objtool: Better handle IRET

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8692ee18-e553-6f90-5291-62d6d57407cd@oracle.com>
Date:   Fri, 17 Apr 2020 19:37:32 +0200
From:   Alexandre Chartre <alexandre.chartre@...cle.com>
To:     Peter Zijlstra <peterz@...radead.org>, tglx@...utronix.de,
        jpoimboe@...hat.com
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, mhiramat@...nel.org,
        mbenes@...e.cz, jthierry@...hat.com
Subject: Re: [PATCH v5 02/17] objtool: Better handle IRET


On 4/16/20 1:47 PM, Peter Zijlstra wrote:
> Teach objtool a little more about IRET so that we can avoid using the
> SAVE/RESTORE annotation. In particular, make the weird corner case in
> insn->restore go away.
> 
> The purpose of that corner case is to deal with the fact that
> UNWIND_HINT_RESTORE lands on the instruction after IRET, but that
> instruction can end up being outside the basic block, consider:
> 
> 	if (cond)
> 		sync_core()
> 	foo();
> 
> Then the hint will land on foo(), and we'll encounter the restore
> hint without ever having seen the save hint.
> 
> By teaching objtool about the arch specific exception frame size, and
> assuming that any IRET in an STT_FUNC symbol is an exception frame
> sized POP, we can remove the use of save/restore hints for this code.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>   arch/x86/include/asm/processor.h |    2 --
>   tools/objtool/arch.h             |    1 +
>   tools/objtool/arch/x86/decode.c  |   14 ++++++++++++--
>   tools/objtool/check.c            |   29 ++++++++++++++++-------------
>   4 files changed, 29 insertions(+), 17 deletions(-)
> 
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -727,7 +727,6 @@ static inline void sync_core(void)
>   	unsigned int tmp;
>   
>   	asm volatile (
> -		UNWIND_HINT_SAVE
>   		"mov %%ss, %0\n\t"
>   		"pushq %q0\n\t"
>   		"pushq %%rsp\n\t"
> @@ -737,7 +736,6 @@ static inline void sync_core(void)
>   		"pushq %q0\n\t"
>   		"pushq $1f\n\t"
>   		"iretq\n\t"
> -		UNWIND_HINT_RESTORE
>   		"1:"
>   		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
>   #endif
> --- a/tools/objtool/arch.h
> +++ b/tools/objtool/arch.h
> @@ -19,6 +19,7 @@ enum insn_type {
>   	INSN_CALL,
>   	INSN_CALL_DYNAMIC,
>   	INSN_RETURN,
> +	INSN_EXCEPTION_RETURN,
>   	INSN_CONTEXT_SWITCH,
>   	INSN_STACK,
>   	INSN_BUG,
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -435,9 +435,19 @@ int arch_decode_instruction(struct elf *
>   		*type = INSN_RETURN;
>   		break;
>   
> +	case 0xcf: /* iret */
> +		*type = INSN_EXCEPTION_RETURN;
> +
> +		/* add $40, %rsp */
> +		op->src.type = OP_SRC_ADD;
> +		op->src.reg = CFI_SP;
> +		op->src.offset = 5*8;
> +		op->dest.type = OP_DEST_REG;
> +		op->dest.reg = CFI_SP;
> +		break;
> +
>   	case 0xca: /* retf */
>   	case 0xcb: /* retf */
> -	case 0xcf: /* iret */
>   		*type = INSN_CONTEXT_SWITCH;
>   		break;
>   
> @@ -483,7 +493,7 @@ int arch_decode_instruction(struct elf *
>   
>   	*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
>   
> -	if (*type == INSN_STACK)
> +	if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
>   		list_add_tail(&op->list, ops_list);
>   	else
>   		free(op);
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2080,15 +2080,14 @@ static int validate_return(struct symbol
>    * tools/objtool/Documentation/stack-validation.txt.
>    */
>   static int validate_branch(struct objtool_file *file, struct symbol *func,
> -			   struct instruction *first, struct insn_state state)
> +			   struct instruction *insn, struct insn_state state)
>   {
>   	struct alternative *alt;
> -	struct instruction *insn, *next_insn;
> +	struct instruction *next_insn;
>   	struct section *sec;
>   	u8 visited;
>   	int ret;
>   
> -	insn = first;
>   	sec = insn->sec;
>   
>   	if (insn->alt_group && list_empty(&insn->alts)) {
> @@ -2141,16 +2140,6 @@ static int validate_branch(struct objtoo
>   				}
>   
>   				if (!save_insn->visited) {
> -					/*
> -					 * Oops, no state to copy yet.
> -					 * Hopefully we can reach this
> -					 * instruction from another branch
> -					 * after the save insn has been
> -					 * visited.
> -					 */
> -					if (insn == first)
> -						return 0;
> -
>   					WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
>   						  sec, insn->offset);
>   					return 1;
> @@ -2243,6 +2232,20 @@ static int validate_branch(struct objtoo
>   
>   			break;
>   
> +		case INSN_EXCEPTION_RETURN:
> +			if (handle_insn_ops(insn, &state))
> +				return 1;

Do we need to update the stack state for normal IRET? This wasn't done before.
So shouldn't this better be:

                case INSN_EXCEPTION_RETURN:
                         if (!func)
                                 return 0;

                         if (handle_insn_ops(insn, &state))
                                 return 1;

                         break;

> +
> +			/*
> +			 * This handles x86's sync_core() case, where we use an
> +			 * IRET to self. All 'normal' IRET instructions are in
> +			 * STT_NOTYPE entry symbols.
> +			 */
> +			if (func)
> +				break;

Is it worth checking that func->name is effectively "sync_core"?

alex.

> +
> +			return 0;
> +
>   		case INSN_CONTEXT_SWITCH:
>   			if (func && (!next_insn || !next_insn->hint)) {
>   				WARN_FUNC("unsupported instruction in callable function",
> 
>