Re: [PATCH] module: reject malformed .modinfo sections

[sashiko-bot@kernel.org]

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 <acharyalaxman8848@gmail.com>

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/20260728173318.63736-1-acharyalaxman8848@gmail.com?part=1