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

[sashiko-bot@kernel.org]

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 <sashal@kernel.org>

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