Re: [RFC PATCH v2 01/11] objtool/klp: Fix checksums and copying of wide strings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] objtool/klp: unadjusted PC-relative string addend in clone_reloc()
--

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

objtool/klp: Fix checksums and copying of wide strings

This commit adds elf_string_len() to compute string lengths in SHF_STRINGS
sections using sh_entsize so that wide strings with embedded zero bytes are
not truncated by strlen() during checksum calculation and livepatch relocation
cloning.

> diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
> index 34da55e456097..6321f691adbbd 100644
> --- a/tools/objtool/klp-diff.c
> +++ b/tools/objtool/klp-diff.c
> @@ -1548,12 +1548,18 @@ static int clone_reloc(struct elfs *e, struct reloc *patched_reloc,
>  	 * version of the string section.  Now copy the string itself.
>  	 */
>  	if (is_string_sec(patched_sym->sec)) {
> -		const char *str = patched_sym->sec->data->d_buf + addend;
> +		ssize_t len = elf_string_len(patched_sym->sec, addend);
> +		const char *str;
>  
> -		__dbg_clone("\"%s\"", escape_str(str));
> +		if (len < 0)
> +			return -1;
> +		str = patched_sym->sec->data->d_buf + addend;
> +		if (patched_sym->sec->sh.sh_entsize == 1)
> +			__dbg_clone("\"%s\"", escape_str(str));
>  
> -		addend = elf_add_string(e->out, out_sym->sec, str);
> -		if (addend == -1)
> +		addend = ALIGN(sec_size(out_sym->sec), out_sym->sec->sh.sh_addralign);
> +		if (!elf_add_data(e->out, out_sym->sec, str,
> +				  len + patched_sym->sec->sh.sh_entsize))
>  			return -1;
>  	}

[Severity: Medium]
What happens in clone_reloc() when a patched function on x86_64 references a
string section via a PC-relative relocation such as R_X86_64_PC32?

In checksum_update_insn(), the relocation addend is adjusted before calling
elf_string_len():

tools/objtool/klp-checksum.c:checksum_update_insn() {
	...
	offset = arch_insn_adjusted_addend(insn, reloc);

	if (is_string_sec(sym->sec)) {
		ssize_t size = elf_string_len(sym->sec, sym->offset + offset);
	...
}

However, in clone_sym_relocs() -> convert_reloc_sym() ->
convert_reloc_sym_to_secsym() -> clone_reloc(), addend comes directly from
reloc_addend(patched_reloc), which still carries the -4 PC-relative bias on
x86_64.

When the target string is at offset 0 of the section (so addend is -4):
- For wide strings (sh_entsize > 1), passing addend as unsigned long offset to
  elf_string_len() wraps to 0xfffffffffffffffcUL, which triggers the
  offset >= sec_size(sec) error and aborts klp diff.
- For single-byte strings (sh_entsize == 1), elf_string_len() and
  elf_add_data() read 4 bytes before patched_sym->sec->data->d_buf.

When the target string is at offset >= 4, elf_string_len() starts 4 bytes
before the string (inside the preceding string's terminator, returning 0 for
4-byte wide strings), copies the wrong bytes, and then overwrites addend with
the aligned output section offset without preserving the PC-relative bias for
elf_create_reloc().

Should clone_reloc() use arch_adjusted_addend(patched_reloc) to locate and
measure the string in patched_sym->sec, and preserve the bias
(reloc_addend(patched_reloc) - arch_adjusted_addend(patched_reloc)) when
computing the new addend for elf_create_reloc()?

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