Re: [PATCH 06/58] objtool/klp: Grow the klp test harness vocabulary

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Symbol names containing dots act as unescaped wildcards in grep assertions, contradicting the commit message's goal.
- [Medium] Missing regex boundary anchors in section_relocs and assert_tombstone allow partial substring or prefix matches, leading to false positives.
--

commit f3b84a0b37e33c6da0d81d19f9f2963fe5db6d2e
Author: Song Liu <song@kernel.org>

objtool/klp: Grow the klp test harness vocabulary

Assertions for what klp diff produces, and for what went into it.

The output side covers the things a livepatch is made of: cloned sections
and symbols, klp symbols and their sympos, tombstones, relocations by
section and by count, the converted .klp.rela sections and their
SHF_RELA_LIVEPATCH flag, SHN_LIVEPATCH symbols, recorded checksums, and klp
diff's own diagnostics -- a rejection for the wrong reason is not a pass.

> diff --git a/tools/objtool/tests/lib.sh b/tools/objtool/tests/lib.sh
> index 0a9da178f4dc8..a4d126e6c9cb3 100644
> --- a/tools/objtool/tests/lib.sh
> +++ b/tools/objtool/tests/lib.sh
[ ... ]
> @@ -458,7 +566,251 @@ assert_patched()
>  
>  assert_not_patched()
>  {
> -	out_sections | grep -q "[[:space:]].text.$1[[:space:]]" &&
> +	out_sections | grep -q "[[:space:]]$(re_quote ".text.$1")[[:space:]]" &&
>  		fail "function '$1' should not have been cloned"
>  	return 0
>  }
> +
> +# section_relocs <section>
> +#
> +# The relocations against one section.  readelf prints every relocation section
> +# in turn, so a test asking about ".smp_locks" has to cut its block out of the
> +# listing first.
> +section_relocs()
> +{
> +	local sec="${1//./\\.}"
> +
> +	out_relocs | awk "/rela$sec/,/^\$/"

[Severity: Medium]
Will this awk range pattern match more sections than intended?

Without a trailing anchor like '/rela$sec$/', searching for a section like
".text" might match and extract relocation blocks for both ".rela.text" and
".rela.text.unlikely".

This could cause assertions like assert_reloc_count() to inadvertently count
entries across all matched blocks.

> +}
> +
> +assert_reloc_sym()
> +{
> +	section_relocs "$1" | grep -qw -- "$2" ||

[Severity: Medium]
Does this grep command need to use re_quote() for the symbol name ($2)?

Since compilers can generate symbol names with dots (e.g., foo.isra.0), leaving
the dot unescaped could allow grep to treat it as a wildcard character,
matching things like "foo_isra_0".

The commit message mentions names are quoted before reaching grep to avoid
this, but it appears some symbol assertions might be missing this quoting.

> +		fail "expected a relocation to '$2' in '$1'"
> +}
[ ... ]
> +assert_klp_sym()
> +{
> +	out_symbols | grep -q "\.klp\.sym\.${2:-[^.]*}\.$1," ||

[Severity: Medium]
Should the symbol name ($1) be escaped here as well?

An unescaped dot in the symbol name could act as a wildcard in this regular
expression, allowing false positive matches in the test harness.

> +		fail "expected klp symbol for '$1'"
> +}
> +
> +# assert_klp_sympos <symbol> <sympos>
> +#
> +# The number after the comma in .klp.sym.<object>.<symbol>,<sympos> says which
> +# of several same-named symbols livepatch should resolve to, counting from 1;
> +# 0 means the name is unique and no disambiguation is needed.  Resolving to the
> +# wrong one is not a load failure, it is a patch quietly wired to the wrong
> +# object.
> +assert_klp_sympos()
> +{
> +	out_symbols | grep -qE "\.klp\.sym\.[^.]+\.$1,$2([[:space:]]|\$)" ||

[Severity: Medium]
Is it possible for an unescaped dot in $1 to cause false matches in this
extended regular expression?

> +		fail "expected klp symbol for '$1' with sympos $2, found:$(
> +			out_symbols | grep -o "\.klp\.sym\.[^.]*\.$1,[0-9]*" |
> +			sort -u | tr '\n' ' ')"
> +}
[ ... ]
> +assert_tombstone()
> +{
> +	out_symbols | grep -q "\.klp\.tombstone\.$1" ||

[Severity: Medium]
Could this pattern match partial symbol names?

Without a trailing word boundary or end-of-line anchor, querying for a
tombstone for "foo" might falsely succeed if a tombstone for "foo_bar"
exists.

Additionally, does $1 need to be escaped here so that dots in the symbol name
are treated literally?

> +		fail "expected a tombstone for '$1'"
> +}
> +
> +assert_symbol()
> +{
> +	out_symbols | grep -qw -- "$1" ||

[Severity: Medium]
Does this direct call to grep need to escape the symbol name?

Like the other assertions, if a symbol name contains a dot, it could act
as an unescaped wildcard character here.

> +		fail "expected symbol '$1' in output"
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911185031.1534046-1-song@kernel.org?part=6