Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] build: missing elf_flagdata() causes relocation updates to be silently discarded - [Medium] build: POSIX exit code truncation wraps unfixed BTI counts of exactly 256 to success - [Medium] scripts: module-bti-check conflates fatal errors with violation counts --
commit f30f3429bd0a90f5e903b1b7fe22a7c8b76f0249 Author: Jeremy Linton <[email protected]> kbuild: modules: Add arm64 BTI fixup utility This commit introduces a new host build tool, module-bti-check, to verify and fix up arm64 cross-section module calls that lack BTI landing pads. It accomplishes this by scanning R_AARCH64_CALL26 relocations and placing veneers in the target section where necessary. > diff --git a/scripts/module-bti-check.c b/scripts/module-bti-check.c > new file mode 100644 > index 0000000000000..c8bef0ed90bb7 > --- /dev/null > +++ b/scripts/module-bti-check.c [ ... ] > if (fix) { > ret = get_veneer(obj, veneers, &target, > &veneer); > if (ret) > goto out; > > rela.r_info = GELF_R_INFO(veneer->secsym, > R_AARCH64_CALL26); > rela.r_addend = veneer->value; > if (!gelf_update_rela(data, j, &rela)) > return elf_error(obj); [Severity: High] Does this miss setting the libelf dirty flag on the data descriptor? When gelf_update_rela() is called in scan_relocations(), it updates the in-memory buffer, but libelf requires elf_flagdata(data, ELF_C_SET, ELF_F_DIRTY) to actually write the modifications back to the file during elf_update(). Without the dirty flag, won't the new landing pad veneers be silently discarded when the updated .rela section is dropped during the file write? > > obj->changed = true; > continue; > } [ ... ] > fprintf(stderr, > "%s: cross-section call from %s%s%s+0x%llx to > %s (%s+0x%llx) lacks a BTI landing pad\n", > obj->name, source ?: "", source ? " " : "", > section_name(obj, shdr.sh_info), > (unsigned long long)rela.r_offset, target.name, > section_name(obj, target.section_index), > (unsigned long long)target.value); > > unfixed++; > } > } > > ret = unfixed; > out: > return ret; [Severity: Medium] Is there a risk of exit code truncation here? scan_relocations() returns the raw count of unfixed occurrences, which is eventually propagated up to main() and used as the process exit code. Since POSIX exit codes are truncated to 8 bits, what happens if the tool encounters exactly 256 (or 512, etc.) unfixed occurrences? It appears returning 256 would wrap to 0, which might trick the build system into treating it as a successful execution despite missing BTI pads. [ ... ] > /* Must happen after elf end, to assure correctness */ > while (veneers) { > struct veneer *next = veneers->next; > > free(veneers); > veneers = next; > } > > if (fd >= 0) > close(fd); > > if (ret < 0) { > if (ret != -ELIBBAD) > fprintf(stderr, "%s: %s\n", name, strerror(-ret)); > return EXIT_FAILURE; > } > > return ret; > } [Severity: Medium] Could this error handling conflate fatal OS or libelf errors with violation counts? In process_file(), negative error codes like -ENOMEM are converted to EXIT_FAILURE (which is 1). Because this overlaps with the value returned when there is exactly 1 unfixed violation, main() cannot differentiate between a tool failure and a BTI warning. [ ... ] > if (elf_version(EV_CURRENT) == EV_NONE) { > fprintf(stderr, "libelf initialization failed: %s\n", > elf_errmsg(-1)); > goto out; > } > > for (i = optind; i < argc; i++) { > int status = process_file(argv[i], fix); > > if (status && !warn) { > ret = status; > goto out; > } > } When the --warn flag is active, it ignores all non-zero status codes. Won't this silently mask build environment failures, returning success even if the file couldn't be opened or processed? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
