Re: [PATCH 17/20] objtool: Fix sibling call detection

[Josh Poimboeuf]

On Thu, Mar 07, 2019 at 12:45:28PM +0100, Peter Zijlstra wrote:
> It turned out that we failed to detect some sibling calls;
> specifically those without relocation records; like:
> 
> $ ./objdump-func.sh defconfig-build/mm/kasan/generic.o __asan_loadN
> 0000 0000000000000840 <__asan_loadN>:
> 0000  840:      48 8b 0c 24             mov    (%rsp),%rcx
> 0004  844:      31 d2                   xor    %edx,%edx
> 0006  846:      e9 45 fe ff ff          jmpq   690 <check_memory_region>
> 
> So extend the cross-function jump to also consider those that are not
> between known (or newly detected) parent/child functions, as
> sibling-cals when they jump to the start of the function.
> 
> The second part of that condition is to deal with random jumps to the
> middle of other function, as can be found in
> arch/x86/lib/copy_user_64.S for example.
> 
> This then (with later patches applied) makes the above recognise the
> sibling call:
> 
> mm/kasan/generic.o: warning: objtool: __asan_loadN()+0x6: call to check_memory_region() with UACCESS enabled
> 
> Also make sure to set insn->call_dest for sibling calls so we can know
> who we're calling.

and we need to know who we're calling because...

>  		if (insn->func && insn->jump_dest->func &&
> -		    insn->func != insn->jump_dest->func &&
> -		    !strstr(insn->func->name, ".cold.") &&
> -		    strstr(insn->jump_dest->func->name, ".cold.")) {
> -			insn->func->cfunc = insn->jump_dest->func;
> -			insn->jump_dest->func->pfunc = insn->func;
> +		    insn->func != insn->jump_dest->func) {
> +
> +			/*
> +			 * For GCC 8+, create parent/child links for any cold
> +			 * subfunctions.  This is _mostly_ redundant with a
> +			 * similar initialization in read_symbols().
> +			 *
> +			 * If a function has aliases, we want the *first* such
> +			 * function in the symbol table to be the subfunction's
> +			 * parent.  In that case we overwrite the
> +			 * initialization done in read_symbols().
> +			 *
> +			 * However this code can't completely replace the
> +			 * read_symbols() code because this doesn't detect the
> +			 * case where the parent function's only reference to a
> +			 * subfunction is through a switch table.
> +			 */
> +			if (!strstr(insn->func->name, ".cold.") &&
> +			    strstr(insn->jump_dest->func->name, ".cold.")) {
> +				insn->func->cfunc = insn->jump_dest->func;
> +				insn->jump_dest->func->pfunc = insn->func;
> +
> +			} else if (insn->jump_dest->func->pfunc != insn->func->pfunc &&
> +				   insn->jump_dest->offset == insn->jump_dest->func->offset) {
> +
> +				/* sibling class */

s/class/call/

> +				insn->call_dest = insn->jump_dest->func;
> +				insn->jump_dest = NULL;
> +			}
>  		}
>  	}
>  
> @@ -1935,9 +1949,16 @@ static int validate_branch(struct objtoo
>  
>  		case INSN_JUMP_CONDITIONAL:
>  		case INSN_JUMP_UNCONDITIONAL:
> -			if (insn->jump_dest &&
> -			    (!func || !insn->jump_dest->func ||
> -			     insn->jump_dest->func->pfunc == func)) {
> +			if (func && !insn->jump_dest) {
> +do_sibling_call:
> +				if (has_modified_stack_frame(&state)) {
> +					WARN_FUNC("sibling call from callable instruction with modified stack frame",
> +							sec, insn->offset);
> +					return 1;
> +				}
> +			} else if (insn->jump_dest &&
> +				   (!func || !insn->jump_dest->func ||
> +				    insn->jump_dest->func->pfunc == func)) {
>  				ret = validate_branch(file, insn->jump_dest,
>  						      state);
>  				if (ret) {
> @@ -1945,25 +1966,17 @@ static int validate_branch(struct objtoo
>  						BT_FUNC("(branch)", insn);
>  					return ret;
>  				}
> -
> -			} else if (func && has_modified_stack_frame(&state)) {
> -				WARN_FUNC("sibling call from callable instruction with modified stack frame",
> -					  sec, insn->offset);
> -				return 1;
>  			}
>  
> -			if (insn->type == INSN_JUMP_UNCONDITIONAL)
> +			if (insn->type == INSN_JUMP_UNCONDITIONAL ||
> +			    insn->type == INSN_JUMP_DYNAMIC)
>  				return 0;
>  
>  			break;
>  
>  		case INSN_JUMP_DYNAMIC:
> -			if (func && list_empty(&insn->alts) &&
> -			    has_modified_stack_frame(&state)) {
> -				WARN_FUNC("sibling call from callable instruction with modified stack frame",
> -					  sec, insn->offset);
> -				return 1;
> -			}
> +			if (func && list_empty(&insn->alts))
> +				goto do_sibling_call;

I would rather have 2-3 duplicated lines of code than complicating the
control flow like this.

-- 
Josh