Re: [RFC PATCH v2 02/11] objtool/klp: Hash compound literals by content

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] objtool/klp: non-inclusive symbol lookup in checksum_update_object()
- [Medium] objtool/klp: out-of-bounds .rodata.cst read in checksum_update_object()
--

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

objtool/klp: Hash compound literals by content

This commit updates klp checksum generation to recursively hash the contents
and relocations of compiler-generated compound literal symbols instead of
hashing their generated symbol names.

> diff --git a/tools/objtool/klp-checksum.c b/tools/objtool/klp-checksum.c
> index efaf083c03a11..2a515c7e2bf60 100644
> --- a/tools/objtool/klp-checksum.c
> +++ b/tools/objtool/klp-checksum.c
[ ... ]
> @@ -134,10 +146,9 @@ static int checksum_update_insn(struct objtool_file *file, struct symbol *func,
>  	sym = reloc->sym;
>  	offset = arch_insn_adjusted_addend(insn, reloc);
>  
> -	if (is_string_sec(sym->sec)) {
> -		ssize_t size = elf_string_len(sym->sec, sym->offset + offset);
> -		const void *data;
> -
> +	if (is_const_reference(sym)) {
> +		size = is_string_sec(sym->sec) ? elf_string_len(sym->sec, sym->offset + offset) :
> +					      sym->sec->sh.sh_entsize;
>  		if (size < 0)
>  			return -1;
>  		data = sym->sec->data->d_buf + sym->offset + offset;
>  		__checksum_update_insn(func, insn, data, size);
>  		goto alts;
>  	}
>  
> -	if (is_anonymous_const_data(sym)) {
> -		void *cst;
> -
> -		cst = sym->sec->data->d_buf + sym->offset + offset;
> -		__checksum_update_insn(func, insn, cst, sym->sec->sh.sh_entsize);
> -		goto alts;
> -	}
> -
>  	if (is_sec_sym(sym)) {
>  		sym = find_symbol_containing(reloc->sym->sec, offset);
>  		if (!sym)
>  			goto alts;
>  
>  		offset -= sym->offset;
>  	}
>  
> -	__checksum_update_insn(func, insn, sym->demangled_name,
> -			       strlen(sym->demangled_name));
> +	if (is_compound_constant(sym)) {
> +		if (checksum_object(file, sym, 1))
> +			return -1;
> +		__checksum_update_insn(func, insn, &sym->csum.checksum,
> +				       sizeof(sym->csum.checksum));
> +	} else {
> +		__checksum_update_insn(func, insn, sym->demangled_name,
> +				       strlen(sym->demangled_name));
> +	}
>  	__checksum_update_insn(func, insn, &offset, sizeof(offset));
>  
>  alts:
> @@ -198,7 +208,8 @@ static int checksum_update_insn(struct objtool_file *file, struct symbol *func,
>  	return 0;
>  }
>  
> -static int checksum_update_object(struct objtool_file *file, struct symbol *sym)
> +static int checksum_update_object(struct objtool_file *file, struct symbol *sym,
> +				  unsigned int depth)
>  {
>  	struct reloc *reloc;
>  
> @@ -212,38 +223,79 @@ static int checksum_update_object(struct objtool_file *file, struct symbol *sym)
>  	sym_for_each_reloc(file->elf, sym, reloc) {
>  		unsigned long sym_offset = reloc_offset(reloc) - sym->offset;
>  		struct symbol *target = reloc->sym;
> +		unsigned int type = reloc_type(reloc);
> +		const void *data;
> +		ssize_t size;
>  		s64 offset;
>  
>  		offset = reloc_addend(reloc);
>  
> -		if (is_string_sec(target->sec)) {
> -			ssize_t size = elf_string_len(target->sec, target->offset + offset);
> -			const void *data;
> +		__checksum_update_object(sym, sym_offset, "reloc type",
> +					 &type, sizeof(type));
>  
> +		if (is_const_reference(target)) {
> +			size = is_string_sec(target->sec) ?
> +				elf_string_len(target->sec, target->offset + offset) :
> +				target->sec->sh.sh_entsize;
>  			if (size < 0)
>  				return -1;
>  			data = target->sec->data->d_buf + target->offset + offset;
> -			__checksum_update_object(sym, sym_offset, "reloc string", data, size);
> +			__checksum_update_object(sym, sym_offset, "reloc constant", data, size);
>  			continue;
>  		}

[Severity: Medium]
Can this read past the end of target->sec->data->d_buf or corrupt the
checksum when a relocation into a .rodata.cst section has a non-zero
intra-entry offset?

When Clang compiles a small constant compound literal array or struct (such
as the 32-byte const unsigned int[8] array in compound.c), it places the
object into a mergeable .rodata.cst section with sh_entsize set to the entry
size and emits relocations against the section symbol.

If a compound literal, static initializer, or instruction references a
non-zero offset within that constant (for example, &arr[1] at offset +4, a
struct member at a non-zero offset, or a one-past-the-end pointer at offset
+sh_entsize), both checksum_update_object() and checksum_update_insn() match
is_const_reference(), set size to sh_entsize, and read a full sh_entsize
bytes starting from d_buf + target->offset + offset without checking section
bounds or aligning to the entry start.

If the referenced constant is the last (or only) entry in the .rodata.cst
section, reading sh_entsize bytes from a non-zero intra-entry offset reads
past the end of d_buf. If another entry follows in the section, the hash
mixes in trailing bytes from the adjacent constant entry while skipping the
leading bytes of the referenced entry, and because this branch returns early
before hashing offset, the intra-entry offset itself is omitted from the
checksum.

>  
>  		if (is_sec_sym(target)) {
>  			target = find_symbol_containing(reloc->sym->sec, offset);
> -			if (!target)
> +			if (!target) {
> +				if (is_compound_constant(sym)) {
> +					ERROR("%s+%#lx: unresolved compound constant reference",
> +					      sym->name, sym_offset);\
> +					return -1;
> +				}
>  				continue;
> +			}
>  
>  			offset -= target->offset;
>  		}

[Severity: Medium]
What happens here when a relocation against a section symbol uses a
one-past-the-end pointer (where offset equals target->offset + target->len),
targets a zero-length local symbol, or references anonymous .rodata?

Unlike convert_reloc_secsym_to_sym() in tools/objtool/klp-diff.c, which
checks find_symbol_by_offset(sec, 0) for dedicated sections, falls back to
find_symbol_containing_inclusive(), and allows anonymous .rodata references,
checksum_update_object() and checksum_update_insn() only call
find_symbol_containing(reloc->sym->sec, offset), which strictly requires
offset to be in [sym->offset, sym->offset + sym->len - 1] and skips
zero-length symbols.

Doesn't that cause two regressions when calculate_checksums() runs:

1. If a compound literal contains a one-past-the-end pointer (even to an
   object in a dedicated -fdata-sections section), a reference to a
   zero-length local symbol, or an anonymous .rodata reference,
   find_symbol_containing() returns NULL and checksum_update_object() aborts
   the build with an unresolved compound constant reference error.

2. If an instruction in checksum_update_insn() or a named object in
   checksum_update_object() references the one-past-the-end address of a
   compound literal (for example, as a loop bound over a compound literal
   array), find_symbol_containing() returns NULL and the compound literal is
   silently omitted from the checksum?

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