Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The `.modinfo` section bypasses bounds validation if its type is set to `SHT_NOBITS`, allowing arbitrary `sh_offset` and `sh_size` values that lead to out-of-bounds kernel memory reads. - [High] The patch fails to protect against out-of-bounds reads if the `.modinfo` section is completely omitted, falling back to an unvalidated `SHT_NULL` section offset. -- --- Patch [1]: [PATCH] module: reject malformed .modinfo sections --- commit 7548fa1ced4767f35fb298eeab3978cae911d641 Author: Laxman Acharya Padhya <[email protected]> module: reject malformed .modinfo sections This commit attempts to prevent loaders from reading past the end of the .modinfo section by rejecting empty sections and those that are not NUL-terminated before parsing them. > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 528690ba160bd..91e20cd1e6ac9 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -2082,6 +2082,8 @@ static int elf_validity_cache_secstrings(struct > load_info *info) > static int elf_validity_cache_index_info(struct load_info *info) > { > int info_idx; > + Elf_Shdr *info_shdr; > + char *modinfo; > > info_idx = find_any_unique_sec(info, ".modinfo"); > [Severity: High] This is a pre-existing issue, but does the early return when info_idx is 0 leave us vulnerable if a module omits the .modinfo section completely? When the subsequent early return for info_idx == 0 is taken, info->index.info remains 0. Subsequent calls to get_modinfo() will use info->sechdrs[0]. While elf_validity_cache_sechdrs() validates the size and addr of section 0, it doesn't validate sh_offset: kernel/module/main.c:elf_validity_cache_sechdrs() if (sechdrs[0].sh_type != SHT_NULL || sechdrs[0].sh_size != 0 || sechdrs[0].sh_addr != 0) { When get_next_modinfo() processes this, it uses this unvalidated sh_offset to calculate modinfo, which is immediately passed to strncmp(): kernel/module/main.c:get_next_modinfo() char *modinfo = (char *)info->hdr + infosec->sh_offset; ... for (p = modinfo; p; p = module_next_tag_pair(p, &size)) { if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=') Could this lead to an out-of-bounds read if sh_offset points outside valid memory? > @@ -2095,6 +2097,18 @@ static int elf_validity_cache_index_info(struct > load_info *info) > } > > info->index.info = info_idx; > + info_shdr = &info->sechdrs[info_idx]; > + modinfo = (char *)info->hdr + info_shdr->sh_offset; > + > + if (info_shdr->sh_size == 0) { > + pr_err("empty .modinfo section\n"); > + return -ENOEXEC; > + } > + if (modinfo[info_shdr->sh_size - 1] != '\0') { [Severity: High] This isn't a bug introduced by this patch, but does accessing modinfo here pose a risk if the .modinfo section type is set to SHT_NOBITS? In elf_validity_cache_sechdrs(), offset validation is explicitly skipped for SHT_NOBITS sections: kernel/module/main.c:elf_validity_cache_sechdrs() case SHT_NOBITS: /* No contents, offset/size don't mean anything */ continue; Since info_shdr->sh_size and sh_offset are unvalidated for these sections, could modinfo[info_shdr->sh_size - 1] read from unmapped or arbitrary memory? > + pr_err(".modinfo section isn't NUL terminated\n"); > + return -ENOEXEC; > + } > + > /* Try to find a name early so we can log errors with a module name */ > info->name = get_modinfo(info, "name"); > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
