Re: [PATCH v8 2/4] kallsyms: extend lineinfo to loadable modules

[sashiko-bot@kernel.org]

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 <sashal@kernel.org>
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/20260801143219.3975824-1-sashal@kernel.org?part=2