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
