Re: [RFC][PATCH 4/7] objtool: Add support for intra-function calls

From: Josh Poimboeuf
Date: Sun Apr 19 2020 - 12:42:05 EST


On Thu, Apr 16, 2020 at 05:07:56PM +0200, Peter Zijlstra wrote:
> From: Alexandre Chartre <alexandre.chartre@xxxxxxxxxx>
>
> Change objtool to support intra-function calls. On x86, an intra-function
> call is represented in objtool as a push onto the stack (of the return
> address), and a jump to the destination address. That way the stack
> information is correctly updated and the call flow is still accurate.
>
> Signed-off-by: Alexandre Chartre <alexandre.chartre@xxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Link: https://lkml.kernel.org/r/20200414103618.12657-4-alexandre.chartre@xxxxxxxxxx
> ---
> include/linux/frame.h | 11 +
> tools/objtool/Documentation/stack-validation.txt | 8 +
> tools/objtool/arch/x86/decode.c | 17 ++-
> tools/objtool/check.c | 129 ++++++++++++++++++-----
> tools/objtool/check.h | 1
> 5 files changed, 140 insertions(+), 26 deletions(-)
>
> --- a/include/linux/frame.h
> +++ b/include/linux/frame.h
> @@ -15,9 +15,20 @@
> static void __used __section(.discard.func_stack_frame_non_standard) \
> *__func_stack_frame_non_standard_##func = func
>
> +/*
> + * This macro indicates that the following intra-function call is valid.
> + * Any non-annotated intra-function call will cause objtool to issue warning.

issue *a* warning

> +static int setup_call_dest(struct objtool_file *file, struct instruction *insn)
> +{
> + unsigned long dest_off;
> +
> + dest_off = insn->offset + insn->len + insn->immediate;
> + insn->call_dest = find_func_by_offset(insn->sec, dest_off);
> + if (!insn->call_dest)
> + insn->call_dest = find_symbol_by_offset(insn->sec, dest_off);
> +
> + if (!insn->call_dest) {
> + /* intra-function call */
> + if (insn->intra_function_call)
> + return 0;
> +
> + WARN_FUNC("intra-function call", insn->sec, insn->offset);
> + return -1;
> + }
> +
> + /* regular call */
> + if (insn->func && insn->call_dest->type != STT_FUNC) {
> + WARN_FUNC("unsupported call to non-function",
> + insn->sec, insn->offset);
> + return -1;
> + }
> +
> + return 0;
> +}
> +

This function should first be added in a separate patch which doesn't
change functionality.

> @@ -2269,6 +2327,29 @@ static int validate_branch(struct objtoo
> if (dead_end_function(file, insn->call_dest))
> return 0;
>
> + if (insn->intra_function_call) {
> + /*
> + * The call instruction can update the stack
> + * state. Then make the intra-function call
> + * behaves like and unconditional jump.

grammar fix: "behave like an unconditional jump".

> + */
> + ret = handle_insn_ops(insn, &state);
> + if (ret)
> + return ret;

validate_branch's callers aren't currently able to handle a negative
return code.

> +
> + ret = validate_branch(file, func, insn,
> + insn->jump_dest, state);
> + if (ret) {
> + if (backtrace) {
> + BT_FUNC("(intra-function call)",
> + insn);
> + }
> + return ret;
> + }
> +
> + return 0;
> + }
> +

Could this be cleaner if the insn->type were just changed to
INSN_JUMP_UNCONDITIONAL? Then it could share the normal jump logic.

Also, now that more instructions are getting stack ops, I wonder if the
ops can just be handled generically for every instruction, with a call
to handle_insn_ops() above, before the switch statement.

--
Josh