| 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
| ||
|
Message-ID: <29b3d533-94cf-4949-90a1-4a8c9d698a8a@redhat.com>
Date: Fri, 23 May 2025 07:46:19 -0400
From: Joe Lawrence <joe.lawrence@...hat.com>
To: Josh Poimboeuf <jpoimboe@...nel.org>, x86@...nel.org
Cc: linux-kernel@...r.kernel.org, Petr Mladek <pmladek@...e.com>,
Miroslav Benes <mbenes@...e.cz>, live-patching@...r.kernel.org,
Song Liu <song@...nel.org>, laokz <laokz@...mail.com>,
Jiri Kosina <jikos@...nel.org>, Marcos Paulo de Souza <mpdesouza@...e.com>,
Weinan Liu <wnliu@...gle.com>, Fazla Mehrab <a.mehrab@...edance.com>,
Chen Zhongjin <chenzhongjin@...wei.com>, Puranjay Mohan <puranjay@...nel.org>
Subject: Re: [PATCH v2 35/62] objtool: Refactor add_jump_destinations()
On 5/9/25 4:16 PM, Josh Poimboeuf wrote:
> The add_jump_destinations() logic is a bit weird and convoluted after
> being incrementally tweaked over the years. Refactor it to hopefully be
> more logical and straightforward.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@...nel.org>
> ---
> tools/objtool/check.c | 227 +++++++++++++---------------
> tools/objtool/include/objtool/elf.h | 4 +-
> 2 files changed, 104 insertions(+), 127 deletions(-)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 66cbeebd16ea..e4ca5edf73ad 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1439,9 +1439,14 @@ static void add_return_call(struct objtool_file *file, struct instruction *insn,
> }
>
> static bool is_first_func_insn(struct objtool_file *file,
> - struct instruction *insn, struct symbol *sym)
> + struct instruction *insn)
> {
> - if (insn->offset == sym->offset)
> + struct symbol *func = insn_func(insn);
> +
> + if (!func)
> + return false;
> +
> + if (insn->offset == func->offset)
> return true;
>
> /* Allow direct CALL/JMP past ENDBR */
> @@ -1449,52 +1454,32 @@ static bool is_first_func_insn(struct objtool_file *file,
> struct instruction *prev = prev_insn_same_sym(file, insn);
>
> if (prev && prev->type == INSN_ENDBR &&
> - insn->offset == sym->offset + prev->len)
> + insn->offset == func->offset + prev->len)
> return true;
> }
>
> return false;
> }
>
> -/*
> - * A sibling call is a tail-call to another symbol -- to differentiate from a
> - * recursive tail-call which is to the same symbol.
> - */
> -static bool jump_is_sibling_call(struct objtool_file *file,
> - struct instruction *from, struct instruction *to)
> -{
> - struct symbol *fs = from->sym;
> - struct symbol *ts = to->sym;
> -
> - /* Not a sibling call if from/to a symbol hole */
> - if (!fs || !ts)
> - return false;
> -
> - /* Not a sibling call if not targeting the start of a symbol. */
> - if (!is_first_func_insn(file, to, ts))
> - return false;
> -
> - /* Disallow sibling calls into STT_NOTYPE */
> - if (is_notype_sym(ts))
> - return false;
> -
> - /* Must not be self to be a sibling */
> - return fs->pfunc != ts->pfunc;
> -}
> -
> /*
> * Find the destination instructions for all jumps.
> */
> static int add_jump_destinations(struct objtool_file *file)
> {
> - struct instruction *insn, *jump_dest;
> + struct instruction *insn;
> struct reloc *reloc;
> - struct section *dest_sec;
> - unsigned long dest_off;
> int ret;
>
> for_each_insn(file, insn) {
> struct symbol *func = insn_func(insn);
> + struct instruction *dest_insn;
> + struct section *dest_sec;
> + struct symbol *dest_sym;
> + unsigned long dest_off;
> + bool dest_undef = false;
> +
> + if (!is_static_jump(insn))
> + continue;
>
> if (insn->jump_dest) {
> /*
> @@ -1503,129 +1488,121 @@ static int add_jump_destinations(struct objtool_file *file)
> */
> continue;
> }
> - if (!is_static_jump(insn))
> - continue;
>
> reloc = insn_reloc(file, insn);
> if (!reloc) {
> dest_sec = insn->sec;
> dest_off = arch_jump_destination(insn);
> - } else if (is_sec_sym(reloc->sym)) {
> + } else if (is_undef_sym(reloc->sym)) {
> + dest_sym = reloc->sym;
> + dest_undef = true;
> + } else {
> dest_sec = reloc->sym->sec;
> - dest_off = arch_insn_adjusted_addend(insn, reloc);
> - } else if (reloc->sym->retpoline_thunk) {
> + dest_off = reloc->sym->sym.st_value +
> + arch_insn_adjusted_addend(insn, reloc);
> + }
> +
> + if (!dest_undef) {
> + dest_insn = find_insn(file, dest_sec, dest_off);
> + if (!dest_insn) {
> + struct symbol *sym = find_symbol_by_offset(dest_sec, dest_off);
> +
> + /*
> + * retbleed_untrain_ret() jumps to
> + * __x86_return_thunk(), but objtool can't find
> + * the thunk's starting RET instruction,
> + * because the RET is also in the middle of
> + * another instruction. Objtool only knows
> + * about the outer instruction.
> + */
> + if (sym && sym->embedded_insn) {
> + add_return_call(file, insn, false);
> + continue;
> + }
> +
> + /*
> + * GCOV/KCOV dead code can jump to the end of
> + * the function/section.
> + */
> + if (file->ignore_unreachables && func &&
> + dest_sec == insn->sec &&
> + dest_off == func->offset + func->len)
> + continue;
> +
> + ERROR_INSN(insn, "can't find jump dest instruction at %s+0x%lx",
> + dest_sec->name, dest_off);
> + return -1;
> + }
> +
> + dest_sym = dest_insn->sym;
> + if (!dest_sym)
> + goto set_jump_dest;
> + }
> +
> + if (dest_sym->retpoline_thunk) {
> ret = add_retpoline_call(file, insn);
> if (ret)
> return ret;
> continue;
> - } else if (reloc->sym->return_thunk) {
> + }
> +
> + if (dest_sym->return_thunk) {
> add_return_call(file, insn, true);
> continue;
> - } else if (func) {
> - /*
> - * External sibling call or internal sibling call with
> - * STT_FUNC reloc.
> - */
> - ret = add_call_dest(file, insn, reloc->sym, true);
> - if (ret)
> - return ret;
> - continue;
> - } else if (reloc->sym->sec->idx) {
> - dest_sec = reloc->sym->sec;
> - dest_off = reloc->sym->sym.st_value +
> - arch_dest_reloc_offset(reloc_addend(reloc));
Way back in ("[PATCH v2 18/62] objtool: Fix x86 addend calculation"),
arch_dest_reloc_offset() was replaced with arch_insn_adjusted_addend(),
so I think that patch missed this callsite and breaks bisectability.
--
Joe
Powered by blists - more mailing lists