Re: [PATCH v6 8/9] selftests/verification: Add tlob selftests

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Using `$(nproc) - 1` to determine a target CPU ID assumes that CPU IDs are contiguous and start from 0, which breaks on sparse affinity masks or offline CPUs.
- [Medium] The ELF parser in `tlob_sym.c` lacks sufficient bounds checks when validating headers and iterating over sections, leading to out-of-bounds memory accesses.
--

--- Patch [8]: [PATCH v6 8/9] selftests/verification: Add tlob selftests ---
commit cb49807af49abc34cb32dee7d7e1d06575cff80a
Author: Wen Yang <[email protected]>

selftests/verification: Add tlob selftests

This commit adds seven ftrace-style test scripts for the tlob RV monitor, 
covering uprobe binding management, budget violation detection, and per-state 
time accounting. It also includes a standalone runner and C helper programs 
for target execution and ELF symbol parsing.

> diff --git a/tools/testing/selftests/verification/test.d/tlob/uprobe_detail_waiting.tc b/tools/testing/selftests/verification/test.d/tlob/uprobe_detail_waiting.tc
> new file mode 100644
> index 0000000000000..798a04012b6ea
> --- /dev/null
> +++ b/tools/testing/selftests/verification/test.d/tlob/uprobe_detail_waiting.tc
[ ... ]
> +start_offset=$("$TLOB_SYM" sym_offset "$UPROBE_TARGET" tlob_preempt_work 2>/dev/null)
> +stop_offset=$("$TLOB_SYM" sym_offset "$UPROBE_TARGET" tlob_preempt_work_done 2>/dev/null)
> +
> +# Pick the last CPU to avoid cpu0 which is used by vng infrastructure.
> +cpu=$(($(nproc) - 1))
> +

[Severity: Medium]
Does this CPU ID calculation safely handle systems with sparse CPU affinity
masks or offline CPUs?

If a system has non-contiguous CPU IDs (for example, if CPU 1 is offline but
CPUs 0 and 2 are online), nproc will evaluate to 2. This calculation would
yield CPU 1, which might cause the subsequent taskset commands to fail with
an invalid argument error since CPU 1 is unavailable.

> +echo 1 > ../events/rv/detail_env_tlob/enable
> +echo 1 > ../tracing_on
> +echo 1 > monitors/tlob/enable
> +echo > ../trace
[ ... ]
> diff --git a/tools/testing/selftests/verification/tlob_sym.c b/tools/testing/selftests/verification/tlob_sym.c
> new file mode 100644
> index 0000000000000..2d9561331d2f9
> --- /dev/null
> +++ b/tools/testing/selftests/verification/tlob_sym.c
[ ... ]
> +		for (int pass = 0; pass < 2 && !found; pass++) {
> +			const char *target = pass ? ".dynsym" : ".symtab";
> +
> +			for (si = 0; si < ehdr->e_shnum && !found; si++) {
> +				Elf64_Shdr *sh = &shdrs[si];
> +				const char *name = shstrtab + sh->sh_name;

[Severity: Medium]
Is there a risk of an out-of-bounds read when accessing the string table
here?

It appears sh->sh_name is used as an offset into shstrtab and passed to
strcmp() without bounds checking against the actual string table size. Could
a malformed binary cause a segmentation fault when the tool attempts to read
beyond the mapped memory?

> +
> +				if (strcmp(name, target) != 0)
> +					continue;
> +
> +				Elf64_Shdr *strtab_sh = &shdrs[sh->sh_link];

[Severity: Medium]
Could this array indexing access memory outside the section header table?

The sh->sh_link field is used directly as an index into shdrs. Since there
is no verification that sh->sh_link is less than ehdr->e_shnum, a malformed
ELF file could cause the tool to read arbitrary memory when mapping headers.

> +				const char *strtab = (char *)map + strtab_sh->sh_offset;
> +				Elf64_Sym *syms = (Elf64_Sym *)((char *)map + sh->sh_offset);
> +				uint64_t nsyms = sh->sh_size / sizeof(Elf64_Sym);
> +				uint64_t j;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8