On 19 September 2026 16:41:39 BST, Fang Xieyan <[email protected]> wrote:
>elf_validity_cache_sechdrs() validates the size and offset of
>every section, unless its type is SHT_NULL or SHT_NOBITS:
>
> switch (shdr->sh_type) {
> case SHT_NULL:
> case SHT_NOBITS:
> /* No contents, offset/size don't mean anything */
> continue;
> default:
> err = validate_section_offset(info, shdr);
>
>elf_validity_cache_index_versions() then reads the extended version
>names by their sh_offset, without checking that the section holds
>data:
>
> if (vers_ext_crc) {
> crc_count = info->sechdrs[vers_ext_crc].sh_size / sizeof(u32);
> name = (void *)info->hdr +
> info->sechdrs[vers_ext_name].sh_offset;
> remaining_len = info->sechdrs[vers_ext_name].sh_size;
> while (crc_count--) {
> name_size = strnlen(name, remaining_len) + 1;
>
>A SHT_NOBITS __version_ext_names reaches here with an sh_offset the
>validator never bounded; past the end of the module the name lookup
>reads out of bounds:
>
> BUG: KASAN: vmalloc-out-of-bounds in strnlen+0x73/0x80
> Read of size 1 at addr ffa00000005534ff by task insmod/79
> ...
> strnlen+0x73/0x80
> load_module+0xef6/0x8600
> The buggy address belongs to a 43-page vmalloc region starting at
> 0xffa0000000529000 allocated at kernel_read_file+0x7b4/0x9f0
>
>This runs after module_sig_check() but before the blacklist check in
>early_mod_check(). Commit 9a5ff4568932 ("module: validate string table
>section types") closed the same class for .shstrtab and .strtab, but
>not this section.
>
>Bounding the offset is not enough: an in-bounds SHT_NOBITS section
>still passes it, and the walk reads unrelated file data as a version
>name. A real __version_ext_names is SHT_PROGBITS, already offset-bounded
>by elf_validity_cache_sechdrs(), so require that type, matching the
>string-table checks.
>
>Fixes: 54ac1ac8edeb ("modules: Support extended MODVERSIONS info")
>Cc: [email protected]
>Assisted-by: Hawkeye:GLM-5.3-flash
>Assisted-by: Qoder:Qwen3.8-Max
>Signed-off-by: Fang Xieyan <[email protected]>
>---
>
>Standalone resubmission of the __version_ext_names fix that was patch 2/2
>of
>"[PATCH v2 0/2] module: close two ELF section-validation gaps" (Message-ID
>[email protected]). The series is split: patch
>1/2
>(the SHT_NULL sh_name fix) is withdrawn because Liu Chao posted a cleaner
>fail-closed fix for the same bug ("[PATCH] module: validate sh_name for
>SHT_NULL sections", [email protected]), which
>drops
>the SHT_NULL special case entirely instead of sanitizing sh_name to 0.
>This
>patch does not depend on it and stands alone.
>
>Changes in v3:
> - Split from the two-patch series and rebased onto mainline; sent as a
> single patch with no cover letter.
> - Corrected the function name in the changelog and the in-code comment:
> elf_validity_check_sectionheaders() does not exist in the tree, the
> validator is elf_validity_cache_sechdrs().
> - Noted where the read sits in load_module(): after module_sig_check() and
> before the blacklist check in early_mod_check(), matching the rationale
> for commit 9a5ff4568932 ("module: validate string table section types"),
> which closed the same class for .shstrtab and .strtab but not this
> section.
> - No change to the check itself: still reject sh_type != SHT_PROGBITS.
>
>v1 bounded the offset with validate_section_offset(). Review pointed out
>that
>is narrower than the bug: a SHT_NOBITS section whose sh_offset is in
>bounds
>still passes the bound, and the walk then reads whatever file bytes sit
>there
>as a version name, which violates ELF semantics (SHT_NOBITS holds no file
>data). v2 requires the section to be SHT_PROGBITS instead, so both the
>out-of-bounds and the in-bounds SHT_NOBITS cases are rejected, while a
>real
>names section (SHT_PROGBITS, already offset-checked) is unaffected.
>
>Reproducer, two variants of a .ko carrying __version_ext_crcs and a
>__version_ext_names of type SHT_NOBITS:
> - sh_offset past the end of the module image. Before: the name walk reads
> out of bounds (KASAN vmalloc-out-of-bounds in strnlen). After: rejected.
> - sh_offset in bounds, aimed at other file data (the harness points it at
> the relocated .shstrtab). Before: the walk silently consumes those bytes
> as a version name and the module is accepted; no splat fires, so a
> crash-only check scores it clean. After: rejected.
>Both variants fail insmod with -ENOEXEC (rc=8, "invalid module format") on
>the
>patched kernel, which is what a module with a malformed version section
>should
>do.
>
>Both cases ran on 704340f1cd0d (9 commits past v7.3-rc3): x86_64 defconfig
>plus
>CONFIG_KASAN_GENERIC and CONFIG_KASAN_VMALLOC, gcc 13.2.0, QEMU under TCG.
>The
>unpatched and patched kernels are built from byte-identical .config files
>and
>differ only by this patch.
>
>LOCALVERSION is pinned so the patched kernel's release string matches the
>payload and the loader reaches the version walk instead of stopping at the
>version magic check.
>
> kernel/module/main.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
>diff --git a/kernel/module/main.c b/kernel/module/main.c
>index d0e1e0b..183e3e2 100644
>--- a/kernel/module/main.c
>+++ b/kernel/module/main.c
>@@ -2287,6 +2287,22 @@ static int elf_validity_cache_index_versions(struct
>load_info *info, int flags)
> * number of entries in every section.
> */
> if (vers_ext_crc) {
>+ /*
>+ * The names section is read below as hdr + sh_offset, so it
>+ * must hold file data. A real one is SHT_PROGBITS.
>+ * elf_validity_cache_sechdrs() exempts SHT_NULL and
>+ * SHT_NOBITS from validate_section_offset() on the assumption
>+ * they have no contents, so a SHT_NOBITS __version_ext_names
>+ * would reach the walk with an offset that was never bounded.
>+ * Require the type; a SHT_PROGBITS section is already bounded
>+ * there, so its sh_offset is safe to dereference.
>+ */
Jesus that comment is huge, did you use AI?
I'm sure I could shorten this
/* sht_nobits skips offset validation so require sht_progbits */
That's it. But this isn't necessarily the "perfect" sort of comment. So it
could be bikeshedded
if (info->sechdrs[vers_ext_name].sh_type != SHT_PROGBITS) {
pr_err("Invalid ELF __version_ext_names type: %u\n",
info->sechdrs[vers_ext_name].sh_type);
return -ENOEXEC;
}
>+ if (info->sechdrs[vers_ext_name].sh_type != SHT_PROGBITS) {
>+ pr_err("Invalid ELF __version_ext_names type: %u\n",
>+ info->sechdrs[vers_ext_name].sh_type);
>+ return -ENOEXEC;
>+ }
>+
> crc_count = info->sechdrs[vers_ext_crc].sh_size / sizeof(u32);
> name = (void *)info->hdr +
> info->sechdrs[vers_ext_name].sh_offset;
>
--- Thanks!
"I'm not a very positive person" - Linus torvalds
