Re: [PATCH] module: validate string table section types

[sashiko-bot@kernel.org]

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 <tweek@google.com>

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/20260708012107.1621513-1-tweek@google.com?part=1