Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Integer overflow in `mod_lineinfo_filenames_off()` and `module_lookup_lineinfo_section()` size calculations bypasses bounds checks, leading to out-of-bounds memory accesses in NMI/panic context. - [Medium] `gen_lineinfo` fails to apply `.debug_info` relocations, causing `dwarf_getsrclines` to read the wrong line program for all CUs except the first in an ET_REL module. --
--- Patch [2]: [PATCH v8 2/4] kallsyms: extend lineinfo to loadable modules --- commit 4466d01d38fe10716b6638bf8ed427b3c7f20a03 Author: Sasha Levin <[email protected]> Subject: kallsyms: extend lineinfo to loadable modules This commit extends CONFIG_KALLSYMS_LINEINFO to loadable kernel modules. It post-processes each .ko at build time to embed a lineinfo section, which is used at runtime to annotate stack traces from module code. > diff --git a/include/linux/mod_lineinfo.h b/include/linux/mod_lineinfo.h > new file mode 100644 > index 0000000000000..75d0bfba6ce57 > --- /dev/null > +++ b/include/linux/mod_lineinfo.h > @@ -0,0 +1,122 @@ [ ... ] > +static inline u32 mod_lineinfo_addrs_off(void) > +{ > + return sizeof(struct mod_lineinfo_header); > +} > + > +static inline u32 mod_lineinfo_file_ids_off(u32 num_entries) > +{ > + return mod_lineinfo_addrs_off() + num_entries * sizeof(u32); > +} > + > +static inline u32 mod_lineinfo_lines_off(u32 num_entries) > +{ > + /* u16 file_ids[] may need 2-byte padding to align lines[] to 4 bytes */ > + u32 off = mod_lineinfo_file_ids_off(num_entries) + > + num_entries * sizeof(u16); > + return (off + 3) & ~3u; > +} > + > +static inline u32 mod_lineinfo_file_offsets_off(u32 num_entries) > +{ > + return mod_lineinfo_lines_off(num_entries) + num_entries * sizeof(u32); > +} > + > +static inline u32 mod_lineinfo_filenames_off(u32 num_entries, u32 num_files) > +{ > + return mod_lineinfo_file_offsets_off(num_entries) + > + num_files * sizeof(u32); > +} [ ... ] > diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c > index f23126d804b25..b0e703e29708b 100644 > --- a/kernel/module/kallsyms.c > +++ b/kernel/module/kallsyms.c [ ... ] > +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) > +{ [ ... ] > + /* > + * Validate counts before multiplying — sizing arithmetic could > + * otherwise overflow on 32-bit with a malformed blob. Each entry > + * contributes one u32 (addrs), one u16 (file_ids), and one u32 > + * (lines); each file contributes one u32 (file_offsets). > + */ > + { > + u32 avail = blob_size - hdr_offset; > + u32 needed = mod_lineinfo_filenames_off(num_entries, num_files); > + > + if (num_entries > U32_MAX / sizeof(u32)) > + return false; > + if (num_files > U32_MAX / sizeof(u32)) > + return false; > + if (needed > avail || filenames_size > avail - needed) > + return false; > + } [Severity: High] Can the needed calculation overflow before the bounds check is evaluated? If num_entries is maliciously large (e.g., 0x33333334), the calculation in mod_lineinfo_filenames_off() sums the array offsets using 32-bit unsigned arithmetic (around 10 bytes per entry). This would wrap around modulo 2^32, resulting in a small needed value that bypasses the needed > avail check. A later binary search on addrs could then access out-of-bounds memory. The division check on num_entries above only guards against a single u32 array's size, not the combined 10 bytes per entry. > diff --git a/scripts/gen_lineinfo.c b/scripts/gen_lineinfo.c > index a3ee54eaad05d..725a8dfbf011a 100644 > --- a/scripts/gen_lineinfo.c > +++ b/scripts/gen_lineinfo.c [ ... ] > +static void apply_debug_line_relocations(Elf *elf) > +{ [ ... ] > + /* Find the relevant sections */ > + while ((scn = elf_nextscn(elf, scn)) != NULL) { > + const char *name; > + > + if (!gelf_getshdr(scn, &shdr)) > + continue; > + name = elf_strptr(elf, shstrndx, shdr.sh_name); > + if (!name) > + continue; > + > + if (!strcmp(name, ".debug_line")) > + debug_line_scn = scn; > + else if (!strcmp(name, ".rela.debug_line")) > + rela_debug_line_scn = scn; > + else if (!strcmp(name, ".rel.debug_line")) > + rel_debug_line_scn = scn; > + else if (shdr.sh_type == SHT_SYMTAB) > + symtab_scn = scn; > + } [Severity: Medium] Does this miss relocations for ET_REL modules that contain multiple object files? This loop collects and applies relocations for .debug_line, but appears to omit .rela.debug_info and .rel.debug_info entirely. Without the relocations applied to .debug_info, the DW_AT_stmt_list attribute for all compile units might remain at offset 0. > +static void process_dwarf(Dwarf *dwarf, unsigned long long text_addr, > + struct covered_section *sections, > + unsigned int num_sections) > { [ ... ] > + if (dwarf_getsrclines(&cudie, &lines, &nlines) != 0) > + goto next; When dwarf_getsrclines() is later called in process_dwarf(), will it repeatedly read the line program of only the first compile unit, leaving most module functions without line info? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
