Re: [PATCH] module: validate sh_name for SHT_NULL sections

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The patch strictly enforces sh_name boundaries on SHT_NULL sections, violating the ELF specification and risking module load failures for valid files.

Pre-existing issues:
- [Critical] The patch leaves unvalidated sh_offset and sh_size fields of SHT_NULL and SHT_NOBITS sections vulnerable, allowing out-of-bounds reads and memory corruption.
- [High] Consumers like find_any_sec, find_any_unique_sec, and module_enforce_rwx_sections still process SHT_NULL sections, which can cause false positives and crashes if undefined fields contain in-bounds garbage.
--

commit 15965760da3d985f09b447439735945868c9d133
Author: Liu Chao [off-list ref]
Subject: module: validate sh_name for SHT_NULL sections

This commit removes the bypass for SHT_NULL sections when checking
section name boundaries. It attempts to prevent out-of-bounds reads
during section name lookups by enforcing that all sections have valid
sh_name offsets.

Link: https://lore.kernel.org/linux-modules/20260708013301.C4D4A1F000E9@smtp.kernel.org/ (local)

[Severity: Medium]
Does enforcing this boundary on SHT_NULL sections violate the ELF
specification?

According to the ELF specification, members of SHT_NULL sections, other
than the sh_type field itself, have undefined values. By removing this
check, we now strictly enforce sh_name is less than strhdr->sh_size for all
sections.

Could this cause a valid ELF module to be incorrectly rejected with
ENOEXEC if it happens to have an undefined but non-zero sh_name left over
from a post-processing tool shrinking the string table?

[Severity: High]
This is a pre-existing issue, but does removing this bypass leave
other SHT_NULL regressions unpatched?

For example, consumers like find_any_sec() and find_any_unique_sec()
do not check if sh_type is SHT_NULL before doing string comparisons:

find_any_unique_sec()
    ...
    if (strcmp(info->secstrings + info->sechdrs[i].sh_name, name) == 0) {

If an attacker crafts an SHT_NULL section with an in-bounds sh_name
that matches a targeted section name like .modinfo, will the loader
erroneously use this empty section and its undefined sh_offset and
sh_size fields?

[Severity: Critical]
This is also a pre-existing issue, but there appears to be a separate path
where unvalidated fields in SHT_NULL sections can lead to memory
corruption.

In elf_validity_cache_sechdrs(), offset and size bounds checking is
explicitly bypassed for SHT_NULL sections:

elf_validity_cache_sechdrs()
    case SHT_NULL:
    case SHT_NOBITS:
        continue;

However, move_module() does not exclude SHT_NULL sections, it only
excludes SHT_NOBITS:

move_module()
    if (shdr->sh_type != SHT_NOBITS) {
        memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
    }

Can this sequence cause an out-of-bounds read from kernel memory if a
crafted module contains an SHT_NULL section with SHF_ALLOC set and an
arbitrary sh_offset?

Similarly, elf_validity_cache_index_mod() accepts SHT_NULL sections for
the .gnu.linkonce.this_module section.

Does this allow info->mod to point to an arbitrary kernel address when
initialized in elf_validity_cache_copy(), leading to out-of-bounds
writes during module initialization?

 		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/20260905110813.150411-1-liuc63@xiaopeng.com?part=1