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
