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
