Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Medium] The patch strictly enforces sh_name boundaries on SHT_NULL sections, violating the ELF specification and risking module load failures for valid files. Pre-existing issues: - [Critical] The patch leaves unvalidated sh_offset and sh_size fields of SHT_NULL and SHT_NOBITS sections vulnerable, allowing out-of-bounds reads and memory corruption. - [High] Consumers like find_any_sec, find_any_unique_sec, and module_enforce_rwx_sections still process SHT_NULL sections, which can cause false positives and crashes if undefined fields contain in-bounds garbage. -- commit 15965760da3d985f09b447439735945868c9d133 Author: Liu Chao <[email protected]> Subject: module: validate sh_name for SHT_NULL sections This commit removes the bypass for SHT_NULL sections when checking section name boundaries. It attempts to prevent out-of-bounds reads during section name lookups by enforcing that all sections have valid sh_name offsets. Link: https://lore.kernel.org/linux-modules/[email protected]/ > diff --git a/kernel/module/main.c b/kernel/module/main.c > index d0e1e0bd2ad06..7d5943cd65432 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -2060,9 +2060,6 @@ static int elf_validity_cache_secstrings(struct > load_info *info) > > for (i = 0; i < info->hdr->e_shnum; i++) { > shdr = &info->sechdrs[i]; > - /* SHT_NULL means sh_name has an undefined value */ > - if (shdr->sh_type == SHT_NULL) > - continue; [Severity: Medium] Does enforcing this boundary on SHT_NULL sections violate the ELF specification? According to the ELF specification, members of SHT_NULL sections, other than the sh_type field itself, have undefined values. By removing this check, we now strictly enforce sh_name is less than strhdr->sh_size for all sections. Could this cause a valid ELF module to be incorrectly rejected with ENOEXEC if it happens to have an undefined but non-zero sh_name left over from a post-processing tool shrinking the string table? [Severity: High] This is a pre-existing issue, but does removing this bypass leave other SHT_NULL regressions unpatched? For example, consumers like find_any_sec() and find_any_unique_sec() do not check if sh_type is SHT_NULL before doing string comparisons: find_any_unique_sec() ... if (strcmp(info->secstrings + info->sechdrs[i].sh_name, name) == 0) { If an attacker crafts an SHT_NULL section with an in-bounds sh_name that matches a targeted section name like .modinfo, will the loader erroneously use this empty section and its undefined sh_offset and sh_size fields? [Severity: Critical] This is also a pre-existing issue, but there appears to be a separate path where unvalidated fields in SHT_NULL sections can lead to memory corruption. In elf_validity_cache_sechdrs(), offset and size bounds checking is explicitly bypassed for SHT_NULL sections: elf_validity_cache_sechdrs() case SHT_NULL: case SHT_NOBITS: continue; However, move_module() does not exclude SHT_NULL sections, it only excludes SHT_NOBITS: move_module() if (shdr->sh_type != SHT_NOBITS) { memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size); } Can this sequence cause an out-of-bounds read from kernel memory if a crafted module contains an SHT_NULL section with SHF_ALLOC set and an arbitrary sh_offset? Similarly, elf_validity_cache_index_mod() accepts SHT_NULL sections for the .gnu.linkonce.this_module section. Does this allow info->mod to point to an arbitrary kernel address when initialized in elf_validity_cache_copy(), leading to out-of-bounds writes during module initialization? > if (shdr->sh_name >= strhdr->sh_size) { > pr_err("Invalid ELF section name in module (section %u > type %u)\n", > i, shdr->sh_type); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
