[PATCH v2 2/2] module: Validate the __version_ext_names section type

[Fang Xieyan <fangxy@xiaopeng.com>]

elf_validity_check_sectionheaders() 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

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, whose offset
elf_validity_check_sectionheaders() already bounds, so require that
type and reject one holding no data, matching the string-table type
checks.

Fixes: 54ac1ac8edeb ("modules: Support extended MODVERSIONS info")
Cc: stable@vger.kernel.org
Assisted-by: Hawkeye:GLM-5.3-flash
Assisted-by: Qoder:Qwen3.8-Max
Signed-off-by: Fang Xieyan <fangxy@xiaopeng.com>
---

Found in the same review as patch 1/2. elf_validity_check_sectionheaders()
exempts SHT_NOBITS from validate_section_offset(), and
elf_validity_cache_index_versions() then dereferences the __version_ext_names
section as hdr + sh_offset without re-checking its type, so a section of that
name and type SHT_NOBITS carries an unvalidated offset into the extended
version name walk. This patch stands on its own; it does not depend on 1/2.

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.

The same vermagic caveat as patch 1/2 applies: 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 e36bfe4..07b9dde 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2296,6 +2296,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_check_sectionheaders() 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.
+		 */
+		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;
-- 
2.50.1 (Apple Git-155)