Re: [RFC PATCH v2 09/11] objtool/klp: Extend twins through unchanged function references

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] objtool/klp: single pass in extend_twins_by_relocations() misses functions
- [Medium] objtool/klp: correlate_reloc() pairs SHN_UNDEF symbols without undef checks
- [Medium] objtool/klp: logical OR guards in correlate_reloc() hide target mismatches
- [Medium] objtool/klp: section-symbol lookup mismatch in correlate_reloc() vs checksum
- [Medium] objtool/klp: same_function_checksum() ignores function alias representative
- [Medium] objtool/klp: correlate_reloc() does not canonicalize data alias symbols
--

commit 1871820aa5fdf039e6c898d41596c9459d2e3e27
Author: Longjun Luo <luolongjuna@gmail.com>

objtool/klp: Extend twins through unchanged function references

This commit extends symbol twin correlation in objtool klp diff by comparing
relocation entries of paired functions that share equal checksums and
correlating their referenced target symbols.

> diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
> index 468b8654b6871..e8252e586260d 100644
> --- a/tools/objtool/klp-diff.c
> +++ b/tools/objtool/klp-diff.c

[ ... ]

> @@ -1294,6 +1297,135 @@ static int convert_reloc_sym(struct elf *elf, struct reloc *reloc)
>  	return convert_reloc_secsym_to_sym(elf, reloc);
>  }
>  
> +static int correlate_reloc(struct elfs *e, struct symbol *func,
> +			   struct reloc *orig_reloc, struct reloc *patched_reloc)
> +{
> +	struct symbol *orig_sym, *patched_sym;
> +	unsigned long offset = reloc_offset(orig_reloc) - func->offset;
> +
> +	if (offset != reloc_offset(patched_reloc) - func->twin->offset ||
> +	    reloc_type(orig_reloc) != reloc_type(patched_reloc)) {
> +		ERROR("%s+%#lx: relocation mismatch in unchanged function", func->name, offset);
> +		return -1;
> +	}
> +
> +	if (reloc_type(orig_reloc) == R_NONE ||
> +	    is_uncorrelated_section(orig_reloc->sym->sec) ||
> +	    is_uncorrelated_section(patched_reloc->sym->sec))
> +		return 0;
> +
> +	/* Section symbols have no twins; resolve the target symbol first. */
> +	if (convert_reloc_secsym_to_sym(e->orig, orig_reloc) ||
> +	    convert_reloc_secsym_to_sym(e->patched, patched_reloc))
> +		return 0;

[Severity: Medium]
Does convert_reloc_secsym_to_sym() resolve one-past-the-end section-symbol
references differently from checksum_update_insn() in
tools/objtool/klp-checksum.c?

In tools/objtool/klp-checksum.c:checksum_update_insn(), section-symbol
relocations are looked up with find_symbol_containing():

	if (is_sec_sym(sym)) {
		sym = find_symbol_containing(reloc->sym->sec, offset);
		if (!sym)
			goto alts;

		offset -= sym->offset;
	}

For a one-past-the-end reference such as arr + ARRAY_SIZE(arr) at
sym->offset + sym->len, find_symbol_containing() returns NULL and skips
hashing both the target symbol identity and the offset.

When extend_twins_by_relocations() subsequently treats the function as
unchanged, convert_reloc_secsym_to_sym() resolves sym->offset + sym->len via
find_symbol_by_offset(sec, 0) or find_symbol_containing_inclusive(). If the
referenced array's size or target changed, won't correlate_reloc() abort
patch generation with a reference offset or target mismatch error instead of
treating the function as changed?

> +
> +	orig_sym = orig_reloc->sym;
> +	patched_sym = patched_reloc->sym;
> +	if (orig_sym->dont_correlate || patched_sym->dont_correlate)
> +		return 0;

[Severity: Medium]
Can these three logical OR guards in correlate_reloc() silently hide
asymmetric relocation targets and conversion errors?

First, if only one side of the relocation pair targets an uncorrelated
section or has dont_correlate set (for example, if a .rodata section-symbol
relocation resolves via find_symbol_containing_inclusive() to a named
STT_OBJECT symbol with dont_correlate == false on one side, while remaining
an unresolved .rodata section symbol with dont_correlate == true on the other
side), returning 0 skips validation and leaves the correlatable symbol
unpaired.

Second, using || in:

	if (convert_reloc_secsym_to_sym(e->orig, orig_reloc) ||
	    convert_reloc_secsym_to_sym(e->patched, patched_reloc))
		return 0;

short-circuits converting patched_reloc when orig_reloc returns non-zero,
ignores cases where only one side fails to resolve a section symbol, and
turns a -1 error return from convert_reloc_secsym_to_sym() into 0.

[Severity: Medium]
Also in correlate_reloc(), what happens when orig_sym or patched_sym is an
SHN_UNDEF symbol?

Unlike correlate_by_identity(), extend_same_twins_by_checksum(), and
validate_symbol_twins(), correlate_reloc() does not check
is_undef_sym(orig_sym) or is_undef_sym(patched_sym), and dont_correlate is
false for SHN_UNDEF symbols. In addition, is_twin_candidate() does not reject
SHN_UNDEF symbols or mismatches in is_undef_sym():

	/* Don't match zero-length labels with sized symbols. */
	if (!is_undef_sym(sym1) && !is_undef_sym(sym2) &&
	    (!sym1->len != !sym2->len))
		return false;

If an unchanged function references an external SHN_UNDEF symbol and
correlate_reloc() sets orig_sym->twin = patched_sym and
patched_sym->twin = orig_sym:

1. In klp_reloc_needed(), reference_twin(patched_sym) becomes non-NULL for an
   unexported undefined symbol, causing klp_reloc_needed() to return true as
   if the symbol were defined in e->orig, which then fails in
   clone_reloc_klp() / find_vmlinux_sympos() or emits a bogus KLP relocation.

2. In clone_reloc_klp(), having orig_sym non-NULL for a weak SHN_UNDEF symbol
   exported by another module skips the !orig_sym check that enforces
   has_module_dep().

3. If a symbol was SHN_UNDEF on one side and defined on the other,
   is_twin_candidate() can pair the undefined symbol (checksum 0) with the
   defined symbol (non-zero checksum), causing mark_changed_functions() to
   report a false changed data error.

Should correlate_reloc() and is_twin_candidate() reject SHN_UNDEF symbols?

> +
> +	if (reloc_addend(orig_reloc) != reloc_addend(patched_reloc)) {
> +		ERROR("%s+%#lx: reference offset mismatch: %s[%u]%+lld and %s[%u]%+lld",
> +		      func->name, offset,
> +		      orig_sym->name, orig_sym->idx, (long long)reloc_addend(orig_reloc),
> +		      patched_sym->name, patched_sym->idx, (long long)reloc_addend(patched_reloc));
> +		return -1;
> +	}
> +
> +	if (orig_sym->twin == patched_sym && patched_sym->twin == orig_sym)
> +		return 0;
> +
> +	if (orig_sym->twin || patched_sym->twin) {
> +		ERROR("%s+%#lx: twin conflict: orig %s[%u]->%s[%u], patched %s[%u]->%s[%u]",
> +		      func->name, offset,
> +		      orig_sym->name, orig_sym->idx,
> +		      orig_sym->twin ? orig_sym->twin->name : "<none>",
> +		      orig_sym->twin ? orig_sym->twin->idx : 0,
> +		      patched_sym->name, patched_sym->idx,
> +		      patched_sym->twin ? patched_sym->twin->name : "<none>",
> +		      patched_sym->twin ? patched_sym->twin->idx : 0);
> +		return -1;
> +	}
> +
> +	/* Compound target checksums are independent of their defining TU. */
> +	if (is_compound_constant(orig_sym) && is_compound_constant(patched_sym)) {
> +		if (!orig_sym->csum.checksum || orig_sym->len != patched_sym->len ||
> +		    orig_sym->csum.checksum != patched_sym->csum.checksum) {
> +			ERROR("%s+%#lx: compound content mismatch in unchanged function",
> +			      func->name, offset);
> +			return -1;
> +		}
> +	} else if (!is_twin_candidate(orig_sym, patched_sym)) {
> +		ERROR("%s+%#lx: incompatible reference targets: %s[%u] and %s[%u]",
> +		      func->name, offset, orig_sym->name, orig_sym->idx,
> +		      patched_sym->name, patched_sym->idx);
> +		return -1;
> +	}
> +
> +	orig_sym->twin = patched_sym;
> +	patched_sym->twin = orig_sym;

[Severity: Medium]
What happens in correlate_reloc() when a direct symbol relocation targets a
non-representative data alias where is_alias_sym(sym) is true?

In tools/objtool/klp-checksum.c:calculate_checksums(), symbols with
is_alias_sym(sym) are skipped, so only the representative symbol sym->alias
has a non-zero csum.checksum, and reference_twin() expects shared twins for
data objects to be stored on sym->alias:

static struct symbol *reference_twin(struct symbol *sym)
{
	if (!sym->twin && is_object_sym(sym))
		return sym->alias->twin;
	return sym->twin;
}

While convert_reloc_secsym_to_sym() resolves section symbols to sym->alias, it
is a no-op when orig_reloc or patched_reloc is already a direct symbol
relocation to a non-representative alias.

If correlate_reloc() operates on non-representative data aliases directly
without canonicalizing STT_OBJECT symbols to sym->alias:

1. Pairing a non-representative alias (csum.checksum == 0) with a
   representative symbol (csum.checksum != 0) causes
   mark_changed_functions() to fail with a false changed data error.

2. Pairing two non-representative aliases leaves twin == NULL on both
   representative sym->alias symbols, so reference_twin(sym->alias) returns
   NULL and mark_changed_functions() compares 0 == 0 on the aliases instead
   of checking the data checksum on sym->alias.

3. If another relocation references the representative sym->alias,
   correlate_reloc() will see a false twin conflict.

[ ... ]

> +static bool same_function_checksum(struct symbol *func)
> +{
> +	/* Cold subfunctions have no separate checksum; use their parent's. */
> +	struct symbol *parent = func->pfunc;
> +	struct symbol *parent_twin = func->twin->pfunc;
> +
> +	return parent->twin == parent_twin && parent->csum.checksum &&
> +	       parent->csum.checksum == parent_twin->csum.checksum;
> +}

[Severity: Medium]
In same_function_checksum(), can parent->csum.checksum be 0 when a function
has an alias at the same address?

In tools/objtool/elf.c:elf_add_symbol(), when two function symbols share the
same section, offset, type, and length, the earlier symbol has sym->alias set
to the later symbol while sym->pfunc remains sym. Because
calculate_checksums() in tools/objtool/klp-checksum.c skips symbols where
is_alias_sym(sym) is true:

	if (is_cold_func(sym) || is_alias_sym(sym) || !sym->len ||
	    !sym->sec || !sym->sec->data)
		continue;

the checksum is only stored on parent->alias->csum.checksum. Won't
same_function_checksum() always evaluate to false for non-representative
function aliases unless it reads the checksum from parent->alias and
parent_twin->alias?

> +
> +static int extend_twins_by_relocations(struct elfs *e)
> +{
> +	struct symbol *sym;
> +
> +	for_each_sym(e->orig, sym) {
> +		if (!is_func_sym(sym) || sym->dont_correlate || !sym->twin ||
> +		    !same_function_checksum(sym))
> +			continue;
> +		if (extend_twin_by_relocations(e, sym))
> +			return -1;
> +	}
> +	return 0;
> +}

[Severity: Medium]
Can a single linear pass in extend_twins_by_relocations() miss functions and
cold subfunctions whose twins are assigned later in the loop?

In ELF symbol tables, all STB_LOCAL symbols (including static helpers,
compiler clones, and .cold subfunctions, which have csum.checksum == 0 and
are not paired by extend_same_twins_by_checksum()) appear before STB_GLOBAL
symbols, and static callees often precede their callers.

When for_each_sym(e->orig, sym) visits an unpaired local function or .cold
subfunction first, !sym->twin (or !same_function_checksum(sym) if its parent
was not yet paired) is true and the loop skips it. When a caller at a later
symbol index is processed by extend_twin_by_relocations() ->
correlate_reloc(), orig_sym->twin = patched_sym is assigned for that earlier
function or .cold subfunction, but extend_twins_by_relocations() never
revisits it to correlate its relocations or verify its references against its
checksum.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104133158.3187525-1-luolongjuna@gmail.com?part=9