Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Unvalidated module blob offsets cause unaligned memory accesses, potentially triggering alignment faults in critical contexts. - [Medium] Integer overflow in the `apply_one_dl_reloc` bounds check allows out-of-bounds heap access in the `gen_lineinfo` tool. - [Low] Dynamically allocated memory for `extra_sections` is never freed in the `gen_lineinfo` tool. --
--- Patch [2]: [PATCH v7 2/4] kallsyms: extend lineinfo to loadable modules --- commit be00704ef1eba0bdfe61160ba94617348dfdc7dc Author: Sasha Levin <[email protected]> kallsyms: extend lineinfo to loadable modules This patch extends the CONFIG_KALLSYMS_LINEINFO feature to loadable kernel modules. It uses scripts/gen-mod-lineinfo.sh to generate .mod_lineinfo sections containing binary tables of source file and line number mappings. At runtime, module_lookup_lineinfo() searches these read-only tables to provide NMI-safe file and line annotations for module stack traces. > diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c > index 0fc11e45df9b9..4888ce8a7f951 100644 > --- a/kernel/module/kallsyms.c > +++ b/kernel/module/kallsyms.c [ ... ] > @@ -494,3 +494,201 @@ int module_kallsyms_on_each_symbol(const char *modname, > +static bool module_lookup_lineinfo_section(const void *blob, u32 blob_size, > + u32 hdr_offset, > + unsigned int section_offset, > + unsigned int min_offset, > + const char **file, > + unsigned int *line) > +{ > + const struct mod_lineinfo_header *hdr; > + const u8 *base; > + const u32 *addrs, *lines, *file_offsets; > + const u16 *file_ids; > + const char *filenames; > + u32 num_entries, num_files, filenames_size; > + unsigned int low, high, mid; > + u16 file_id; > + > + if (hdr_offset > blob_size || > + blob_size - hdr_offset < sizeof(*hdr)) > + return false; > + > + base = (const u8 *)blob + hdr_offset; > + hdr = (const struct mod_lineinfo_header *)base; [Severity: High] Does this code safely handle unaligned memory accesses? The hdr_offset value is read directly from the binary blob and is never validated for 4-byte alignment before being cast to struct mod_lineinfo_header * and subsequently u32 * for the internal arrays. If a malformed or corrupted .ko file provides an unaligned hdr_offset, dereferencing these pointers could trigger a hardware alignment trap on strict-alignment architectures. Since this code is designed to be called during NMI or panic stack traces, taking an alignment fault here would cause a recursive fault and hang the system without producing the crash dump. Could we add an alignment check for hdr_offset before casting it? [ ... ] > diff --git a/scripts/gen_lineinfo.c b/scripts/gen_lineinfo.c > index 699e760178f09..7624098d0cb42 100644 > --- a/scripts/gen_lineinfo.c > +++ b/scripts/gen_lineinfo.c [ ... ] > +static void apply_one_dl_reloc(Elf_Data *dl_data, Elf_Data *sym_data, > + bool target_le, > + unsigned int abs32_type, unsigned int abs64_type, > + GElf_Xword r_info, GElf_Addr r_offset, > + GElf_Sxword addend, bool has_addend) > +{ > + GElf_Sym sym; > + unsigned int r_type = GELF_R_TYPE(r_info); > + size_t r_sym = GELF_R_SYM(r_info); > + bool is_abs64; > + size_t width; > + uint64_t value; [ ... ] > + width = is_abs64 ? 8 : 4; > + > + if (r_offset + width > dl_data->d_size) > + return; [Severity: Medium] Can this bounds check overflow? Because r_offset is a 64-bit integer, a maliciously crafted .ko file fed to the gen_lineinfo tool could provide an extremely large value (such as 0xFFFFFFFFFFFFFFFE). Adding width to it would wrap around to a small number, successfully bypassing the bounds check. This could lead to out-of-bounds pointer arithmetic and a subsequent out-of-bounds write in elf_write_uint() below, potentially causing a crash or code execution within the kernel build environment. [ ... ] > +static void resolve_covered_sections(Elf *elf, > + struct covered_section *sections, > + unsigned int num_sections) > +{ [ ... ] > + if (!covered && > + (shdr.sh_flags & SHF_EXECINSTR) && (shdr.sh_flags & > SHF_ALLOC) && > + shdr.sh_size && shdr.sh_size <= UINT_MAX) { > + struct covered_section *es; > + > + extra_sections = realloc(extra_sections, > + (num_extra_sections + 1) * > + sizeof(*extra_sections)); [ ... ] > @@ -552,6 +1387,5 @@ int main(int argc, char *argv[]) > for (unsigned int i = 0; i < num_files; i++) > free(files[i].name); > free(files); > - > return 0; > } [Severity: Low] Does this code leak the dynamically allocated extra_sections array? In resolve_covered_sections(), extra_sections is allocated and expanded via realloc(). However, the cleanup block in main() only frees entries and files, missing the extra_sections array. While this is a short-lived userspace tool and the OS will automatically reclaim the memory upon exit, would it be better to explicitly free extra_sections to keep the cleanup complete? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
