Re: [PATCH v3 3/3] module/kallsyms: Filter out local and mapping symbols during module load

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Relocations can modify `.symtab` and `.strtab` between `layout_symtab()` and `add_kallsyms()`, leading to a heap buffer overflow, out-of-bounds read, and security control bypass.
--

--- Patch [3]: [PATCH v3 3/3] module/kallsyms: Filter out local and mapping symbols during module load ---
commit b99e0a63da9d8d5ffb4bf7cbd78e368abde75865
Author: Tiezhu Yang <[email protected]>

module/kallsyms: Filter out local and mapping symbols during module load

The compiler toolchains generate local and mapping symbols on certain
architectures like LoongArch for optimizations and relocations. This
commit filters them out during module load time using the newly
introduced is_kept_symbol() helper to keep /proc/kallsyms clean.

> diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
> index efc73f66defd5..77e05b5ca1f2c 100644
> --- a/kernel/module/kallsyms.c
> +++ b/kernel/module/kallsyms.c
> @@ -89,6 +89,9 @@ static bool is_kept_symbol(struct module *mod, const struct load_info *info,
>  	    !src->st_name)
>  		return false;
>  
> +	if (is_local_mapping_symbol(&info->strtab[src->st_name]))
> +		return false;
> +

[Severity: High]
This is a pre-existing issue, but does this new check introduce an
out-of-bounds read if a malformed module modifies st_name?

In layout_symtab(), .symtab and .strtab are marked with SHF_ALLOC:

kernel/module/kallsyms.c:layout_symtab() {
    ...
    symsect->sh_flags |= SHF_ALLOC;
    strsect->sh_flags |= SHF_ALLOC;
    ...
}

Later, apply_relocations() applies relocations to sections with
SHF_ALLOC, which can mutate the symbol and string tables in module memory:

kernel/module/main.c:apply_relocations() {
    ...
    if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC) &&
        (!infosec || infosec != info->index.pcpu))
        continue;
    ...
}

When add_kallsyms() re-evaluates the mutated symbol table, it calls
is_kept_symbol(), which uses the mutated src->st_name without bounds
checking against the unmodified info->strtab.

Could this read out of bounds if st_name is maliciously inflated?

Also, since a previously rejected symbol might now be kept after mutation,
could this overflow the mod->core_kallsyms.symtab array since the number
of kept symbols might now exceed the pre-calculated allocation?

>  #ifdef CONFIG_KALLSYMS_ALL
>  	if (src->st_shndx == info->index.pcpu)
>  		return true;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3