Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] scripts/module-bti-check: missing elf_flagdata() on relocation buffer - [High] kbuild: module-bti-check ignores R_AARCH64_JUMP26 tail calls causing BTI panics - [Medium] kbuild: unconditional bti_check execution breaks incremental builds --
commit 6f17af1f0f493574af3f26ef3b98809bf3a9aca7 Author: Jeremy Linton <[email protected]> kbuild: modules: Build and trigger BTI scanner for arm64 Now that we have a utility which can detect and correct missing landing pads, build it and hook it up to scan during module finalization. > diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal > index 01a37ec872b90..973adfa72a7d4 100644 > --- a/scripts/Makefile.modfinal > +++ b/scripts/Makefile.modfinal [ ... ] > @@ -47,8 +53,12 @@ quiet_cmd_btf_ko = BTF [M] $@ > fi; > > # Re-generate module BTFs if either module's .ko or vmlinux changed > -%.ko: %.o %.mod.o .module-common.o $(objtree)/scripts/module.lds $(and > $(CONFIG_DEBUG_INFO_BTF_MODULES),$(KBUILD_BUILTIN),$(objtree)/vmlinux) FORCE > +%.ko: %.o %.mod.o .module-common.o $(objtree)/scripts/module.lds > $(bti_check_dep) $(and > $(CONFIG_DEBUG_INFO_BTF_MODULES),$(KBUILD_BUILTIN),$(objtree)/vmlinux) FORCE > +$(call if_changed,ld_ko_o) > +ifdef CONFIG_ARM64_BTI_KERNEL > + +$(call cmd,bti_check) > +endif > + > ifdef CONFIG_DEBUG_INFO_BTF_MODULES > +$(if $(newer-prereqs),$(call cmd,btf_ko)) > endif [Severity: Medium] Will this execute the bti_check unconditionally on every incremental build? Since the %.ko target uses FORCE, Kbuild relies on conditional macros to prevent redundant execution. The bti_check command is not wrapped in an if_changed guard. Could this cause module-bti-check --fix to run on all modules every time Kbuild is invoked, breaking the fast path for null builds? While reviewing this, I also noticed a couple of regressions in scripts/module-bti-check.c: scripts/module-bti-check.c:scan_relocations() { ... if (!gelf_getrela(data, j, &rela)) goto out_elferr; if (GELF_R_TYPE(rela.r_info) != R_AARCH64_CALL26) continue; ... } [Severity: High] Are R_AARCH64_JUMP26 tail calls intentionally filtered out here? If a module contains a cross-section tail call to a static function, and the distance requires a PLT, the PLT entry will use an indirect branch to target the function. Since static targets don't emit their own BTI pads and the JUMP26 relocation is bypassed here, wouldn't this indirect branch trigger a BTI exception and a kernel panic at runtime? scripts/module-bti-check.c:scan_relocations() { ... 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); obj->changed = true; continue; ... } [Severity: High] Are these modified relocation entries successfully written back to the file? gelf_update_rela() modifies the Elf_Data relocation block in memory, but the data block is never flagged as dirty with elf_flagdata(). According to libelf semantics, won't elf_update() silently discard these relocation changes because the buffer isn't marked dirty? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
