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.
+ */
+ 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)
