Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] module: OOB memory read via SHT_NULL section aliasing --
commit 536b936ee0704f1857141d93c7d4113160b708c4 Author: Fang Xieyan <[email protected]> module: Sanitize the undefined sh_name of SHT_NULL sections This commit sets the sh_name of SHT_NULL sections to 0 in order to provide a safe, in-bounds value for section name walkers. By giving undefined values the empty string at index 0, valid modules remain unaffected while out-of-bounds reads are prevented. > diff --git a/kernel/module/main.c b/kernel/module/main.c > index d0e1e0bd2ad06..e36bfe4734d59 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -2060,9 +2060,18 @@ 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) > + /* > + * SHT_NULL means sh_name has an undefined value. The section > + * name walkers that follow (find_any_unique_sec(), > + * module_mark_ro_after_init(), ...) look the name up as > + * secstrings + sh_name for every section, so give the undefined > + * value a safe in-bounds meaning instead of skipping the check: > + * the empty string at index 0. > + */ > + if (shdr->sh_type == SHT_NULL) { > + shdr->sh_name = 0; [Severity: High] Does this safely neutralize SHT_NULL sections? This forcefully sets the name to index 0, assuming index 0 is always guaranteed to be an empty string. However, earlier in elf_validity_cache_secstrings(), the string table validation checks the last byte but fails to ensure that secstrings[0] == '\0': kernel/module/main.c:elf_validity_cache_secstrings() { ... if (secstrings[strhdr->sh_size - 1] != '\0') { pr_err("ELF Spec violation: section name table isn't null terminated\n"); return -ENOEXEC; } ... } Because of this, can an attacker craft a string table starting with a critical section name like .modinfo so that a SHT_NULL section at index i > 0 aliases to .modinfo? Since walkers like find_any_unique_sec() do not filter out SHT_NULL sections, this spoofed name at index 0 could be successfully matched: kernel/module/main.c:find_any_unique_sec() { ... for (i = 1; i < info->hdr->e_shnum; i++) { if (strcmp(info->secstrings + info->sechdrs[i].sh_name, name) == 0) { ... } Furthermore, SHT_NULL sections are explicitly exempted from offset validation in elf_validity_cache_sechdrs(): kernel/module/main.c:elf_validity_cache_sechdrs() { ... switch (shdr->sh_type) { case SHT_NULL: case SHT_NOBITS: /* No contents, offset/size don't mean anything */ continue; ... } Could this allow functions like get_next_modinfo() to dereference the unvalidated sh_offset of the aliased SHT_NULL section? kernel/module/main.c:get_next_modinfo() { ... char *modinfo = (char *)info->hdr + infosec->sh_offset; ... } Could this lead to an out-of-bounds memory read and a potential kernel panic? > continue; > + } > 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
