Re: [PATCH v3 2/3] module/kallsyms: Refactor current symbol filtering into is_kept_symbol()

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

Pre-existing issues:
- [High] An attacker can use ELF relocations targeting the `.symtab` section to bypass early validation checks. This leads to heap buffer overflows, out-of-bounds reads, and information leaks during module loading.
--

--- Patch [2]: [PATCH v3 2/3] module/kallsyms: Refactor current symbol filtering into is_kept_symbol() ---
commit a466dcd66d6b608624bbdcca606a5281e2967615
Author: Tiezhu Yang <[email protected]>

module/kallsyms: Refactor current symbol filtering into is_kept_symbol()

Currently, the loops in layout_symtab() and add_kallsyms() duplicate the
exact same checks to determine whether a symbol should be preserved in
the module's core symbol table. This commit renames is_core_symbol() to
is_kept_symbol() and consolidates the checking logic.

> diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
> index 8955cd42f20bc..efc73f66defd5 100644
> --- a/kernel/module/kallsyms.c
> +++ b/kernel/module/kallsyms.c

[ ... ]

> @@ -197,23 +199,21 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
>  	strtab_size = info->core_typeoffs - info->stroffs;
>  	src = kallsyms->symtab;
>  	for (ndst = i = 0; i < kallsyms->num_symtab; i++) {
> +		ssize_t ret;
> +
>  		kallsyms->typetab[i] = elf_type(src + i, info);
> -		if (i == 0 || is_livepatch_module(mod) ||
> -		    is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
> -				   info->index.pcpu)) {
> -			ssize_t ret;
> -
> -			mod->core_kallsyms.typetab[ndst] =
> -				kallsyms->typetab[i];
> -			dst[ndst] = src[i];
> -			dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
> -			ret = strscpy(s, &kallsyms->strtab[src[i].st_name],
> -				      strtab_size);
> -			if (ret < 0)
> -				break;
> -			s += ret + 1;
> -			strtab_size -= ret + 1;
> -		}
> +
> +		if (!is_kept_symbol(mod, info, i, src + i))
> +			continue;
> +
> +		mod->core_kallsyms.typetab[ndst] = kallsyms->typetab[i];
> +		dst[ndst] = src[i];
> +		dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
> +		ret = strscpy(s, &kallsyms->strtab[src[i].st_name], strtab_size);

[Severity: High]
This isn't a bug introduced by this patch, but could an attacker use ELF
relocations targeting the .symtab section to bypass early validation checks
and trigger a heap buffer overflow or out-of-bounds read here?

During module loading, layout_symtab() adds the SHF_ALLOC flag to the
.symtab section and computes memory requirements based on the unmodified
symbols:

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

Later, apply_relocations() processes relocation sections. Because .symtab
now has the SHF_ALLOC flag, relocations targeting .symtab are applied,
modifying the symbol table in memory after bounds checks have completed:

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

Finally, add_kallsyms() reads this modified symbol table without
re-validating it. If an attacker modifies st_name via relocations, it
bypasses the earlier elf_validity_cache_strtab() bounds check, causing
strscpy() to read out-of-bounds memory. Modifying st_shndx bypasses the
ndst calculation, causing out-of-bounds writes on mod->core_kallsyms.symtab
and typetab arrays.

> +		if (ret < 0)
> +			break;
> +		s += ret + 1;
> +		strtab_size -= ret + 1;
>  	}
>  
>  	/* Set up to point into init section. */

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