Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] The patch fixes an out-of-bounds read vulnerability for string tables by ensuring they are of type SHT_STRTAB, but fails to address the exact same vulnerability class for several other ELF sections (e.g., .modinfo, .gnu.linkonce.this_module, __version_ext_names, __ksymtab). - [High] Iterating over sections in lookup functions triggers an out-of-bounds string read for SHT_NULL sections due to missing sh_name bounds checking. - [High] Validation logic for section headers accesses sechdrs[0] out of bounds if e_shnum is 0 and e_shoff is near the end of the file. -- --- Patch [1]: [PATCH] module: validate string table section types --- commit e8e815fedf365febe70f5870d807616d37946281 Author: Thiébaud Weksteen <[email protected]> module: validate string table section types In elf_validity_cache_sechdrs, section sizes and offsets are validated, unless the section type is SHT_NULL or SHT_NOBITS. Later, elf_validity_cache_secstrings and elf_validity_cache_index_str access the section name table (.shstrtab) and symbol string table (.strtab) headers without first ensuring that their types are SHT_STRTAB. If a section type is SHT_NULL or SHT_NOBITS, sh_offset has not been validated and may reference out-of-bounds memory when dereferenced in elf_validity_cache_secstrings or elf_validity_cache_strtab. Validate that both string section headers are of type SHT_STRTAB before caching them. > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 46dd8d25a6058..7cbc8f0e28c61 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -2011,6 +2011,7 @@ static int elf_validity_cache_sechdrs(struct load_info > *info) [Severity: High] This is a pre-existing issue, but does the validation logic in elf_validity_cache_sechdrs() access sechdrs out of bounds if e_shnum is 0 and e_shoff is near the end of the file? The overflow check evaluates to 0 > positive if e_shnum is 0 and e_shoff is less than info->len, allowing the check to incorrectly pass: elf_validity_cache_sechdrs() { ... if (info->hdr->e_shoff >= info->len || (info->hdr->e_shnum * sizeof(Elf_Shdr) > info->len - info->hdr->e_shoff)) { ... } The code then sets sechdrs and blindly accesses sechdrs[0], reading past the end of the user-provided module buffer: elf_validity_cache_sechdrs() { ... if (sechdrs[0].sh_type != SHT_NULL || sechdrs[0].sh_size != 0 || sechdrs[0].sh_addr != 0) { ... } > * Specifically checks: > * > * * Section name table index is inbounds of section headers > + * * Section name table type is SHT_STRTAB > * * Section name table is not empty > * * Section name table is NUL terminated [ ... ] > @@ -2038,6 +2039,11 @@ static int elf_validity_cache_secstrings(struct > load_info *info) > > strhdr = &info->sechdrs[info->hdr->e_shstrndx]; > > + if (strhdr->sh_type != SHT_STRTAB) { > + pr_err("Invalid ELF section name table type: %u\n", > strhdr->sh_type); > + return -ENOEXEC; > + } > + [Severity: High] This is a pre-existing issue, but does iterating over sections in lookup functions trigger an out-of-bounds string read for SHT_NULL sections due to missing sh_name bounds checking? In elf_validity_cache_secstrings(), bounds validation of sh_name against the string table size is explicitly skipped for SHT_NULL sections: elf_validity_cache_secstrings() { ... if (shdr->sh_type == SHT_NULL) continue; ... } Later, section lookup functions like find_any_unique_sec() iterate over all sections and blindly pass the unvalidated info->secstrings pointer to strcmp(): find_any_unique_sec() { ... if (strcmp(info->secstrings + info->sechdrs[i].sh_name, name) == 0) { ... } Could this cause an immediate out-of-bounds memory read when strcmp() executes if an attacker sets a huge sh_name on a SHT_NULL section? [ ... ] > @@ -2218,6 +2224,12 @@ static int elf_validity_cache_index_str(struct > load_info *info) > return -ENOEXEC; > } > > + if (info->sechdrs[str_idx].sh_type != SHT_STRTAB) { > + pr_err("Invalid ELF symbol string table type: %u\n", > + info->sechdrs[str_idx].sh_type); > + return -ENOEXEC; > + } > + [Severity: High] This is a pre-existing issue, but while this patch fixes an out-of-bounds read vulnerability for string tables by ensuring they are of type SHT_STRTAB, does the same vulnerability class exist for other ELF sections? Since elf_validity_cache_sechdrs() skips bounds checking of sh_offset and sh_size for SHT_NULL and SHT_NOBITS sections, lookup functions can match sections by name while ignoring sh_type. For example, elf_validity_cache_index_info() looks up .modinfo by name and caches it without validating its sh_type: elf_validity_cache_index_info() { ... info_idx = find_any_unique_sec(info, ".modinfo"); ... } Similarly, in elf_validity_cache_index_mod(), the .gnu.linkonce.this_module section rejects SHT_NOBITS but fails to reject SHT_NULL: elf_validity_cache_index_mod() { ... if (shdr->sh_type == SHT_NOBITS) { ... } And in move_module(), the loop skips SHT_NOBITS but calls memcpy for SHT_NULL: move_module() { ... memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size); ... } Could a malicious module supply these sections with type SHT_NULL and an arbitrary, unvalidated sh_offset, leading to out-of-bounds reads or copies? > info->index.str = str_idx; > return 0; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
