Re: [PATCH v5 08/11] objtool: size the instruction hash to the text

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] objtool: unguarded NULL file->insn_hash dereference in find_insn()
--

commit 213d34d383fb04e85ef5c721bf77b5b42b0870d5
Author: Lorenzo Stoakes (ARM) <ljs@kernel.org>

objtool: size the instruction hash to the text

This commit replaces the fixed-size instruction hash table in
struct objtool_file with a dynamically allocated hash table sized
according to the total text section size in decode_instructions().

> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 931c5c412f15d..97250aa361009 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -38,12 +38,22 @@ struct disas_context *objtool_disas_ctx;
>  
>  size_t sym_name_max_len;
>  
> +static struct hlist_head *insn_hash_head(struct objtool_file *file,
> +					 struct section *sec, unsigned long offset)
> +{
> +	/* Determine instruction hash based on section index and offset. */
> +	const u32 sec_hash = sec_offset_hash(sec, offset);
> +	const u32 hash = hash_min(sec_hash, file->insn_hash_bits);
> +
> +	return &file->insn_hash[hash];
> +}
> +
>  struct instruction *find_insn(struct objtool_file *file,
>  			      struct section *sec, unsigned long offset)
>  {
>  	struct instruction *insn;
>  
> -	hash_for_each_possible(file->insn_hash, insn, hash, sec_offset_hash(sec, offset)) {
> +	hlist_for_each_entry(insn, insn_hash_head(file, sec, offset), hash) {
>  		if (insn->sec == sec && insn->offset == offset)
>  			return insn;
>  	}

[Severity: Medium]
As reported by Sashiko on v4 [1], can find_insn() dereference a NULL
file->insn_hash pointer if decode_file() fails before reaching
decode_instructions()?

With this change, file->insn_hash is NULL and file->insn_hash_bits is 0
until decode_instructions() calls alloc_insn_hash(). However, in
decode_file(), init_pv_ops() runs before decode_instructions():

tools/objtool/check.c:decode_file() {
	if (init_pv_ops(file))
		return -1;

	/*
	 * Must be before add_{jump_call}_destination.
	 */
	if (classify_symbols(file))
		return -1;

	if (decode_instructions(file))
		return -1;
}

If init_pv_ops() -> add_pv_ops() returns -1 (for instance, when
find_symbol_by_offset() returns NULL for a pv_ops table relocation),
check() and cmd_klp_checksum() jump to out: and unconditionally call
free_insns(file) (as well as disas_funcs() when opts.disas is set).

free_insns() then walks all sections via for_each_insn() ->
sec_for_each_insn() -> find_insn(), calling insn_hash_head() with
file->insn_hash == NULL and file->insn_hash_bits == 0, which performs a
32-bit shift by 32 in hash_32() and dereferences NULL in find_insn().

Should find_insn() (or free_insns()) check if (!file->insn_hash) before
looking up instructions?

[1] https://lore.kernel.org/r/20260923-build-speedup-v4-18-73128809a4a4@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-build-speedup-v5-0-0f6018be8dae@kernel.org?part=8