[PATCH v2 0/2] module: close two ELF section-validation gaps

[PATCH v2 0/2] module: close two ELF section-validation gaps

[Fang Xieyan]

Both patches come from the same review of the module ELF validator, and
both fix an out-of-bounds read of a section field that the validator
leaves unchecked on purpose.

elf_validity_check_sectionheaders() exempts SHT_NULL and SHT_NOBITS
sections from its bounds checks, on the assumption that a section with
no contents has nothing to dereference. The loaders that run after it do
not share that assumption: they read sh_name and sh_offset for every
section, so an exempted section carries an unvalidated field into a
string lookup or a pointer computation. Commit 9a5ff4568932 ("module:
validate string table section types") closed two such gaps for
.shstrtab and .strtab; these two patches close the ones that remain.

Patch 1 gives the undefined sh_name of a SHT_NULL section a safe value,
so the section walkers that follow read the empty string instead of
running past .shstrtab.

Patch 2 requires the __version_ext_names section to be SHT_PROGBITS
before elf_validity_cache_index_versions() dereferences its sh_offset,
so a SHT_NOBITS section of that name - which the validator exempts and
which holds no file data - cannot drive the extended version name walk
outside the module image or make it read unrelated bytes as a name.

Each patch stands alone and can be applied or backported separately;
they are grouped only because they share a root cause. Both were
reproduced on 704340f1cd0d with KASAN, and each is fixed by its own
change.

Changes in v2. v1 was posted here:
https://lore.kernel.org/all/20260916174753.14870-1-fangxy@xiaopeng.com/

 - Patch 2: require __version_ext_names to be SHT_PROGBITS before its
   sh_offset is dereferenced, instead of bounding the offset. Review
   pointed out the offset bound is narrower than the bug: an in-bounds
   SHT_NOBITS section passes it and the walk then reads unrelated file
   bytes as a version name. The type check rejects both the
   out-of-bounds and the in-bounds variants; the reproducer now covers
   both. Details in the patch's tear-line notes.
 - Patch 1: unchanged since v1.

Fang Xieyan (2):
  module: Sanitize the undefined sh_name of SHT_NULL sections
  module: Validate the __version_ext_names section type

 kernel/module/main.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

-- 
2.50.1 (Apple Git-155)

[PATCH v2 1/2] module: Sanitize the undefined sh_name of SHT_NULL sections

[Fang Xieyan]

elf_validity_cache_secstrings() skips the sh_name bounds check for a
SHT_NULL section:

	for (i = 0; i < info->hdr->e_shnum; i++) {
		shdr = &info->sechdrs[i];
		/* SHT_NULL means sh_name has an undefined value */
		if (shdr->sh_type == SHT_NULL)
			continue;
		if (shdr->sh_name >= strhdr->sh_size) {

A SHT_NULL section may then carry an sh_name past the end of
.shstrtab. The name lookups that run afterwards do not skip SHT_NULL.
This is find_any_unique_sec(), which load_module() uses to locate
".modinfo":

	for (i = 1; i < info->hdr->e_shnum; i++) {
		if (strcmp(info->secstrings + info->sechdrs[i].sh_name,
			   name) == 0) {

so the out-of-bounds sh_name is read as a string. A module carrying a
crafted SHT_NULL section reads past its in-memory copy:

  BUG: KASAN: vmalloc-out-of-bounds in strcmp+0xb0/0xc0
  Read of size 1 at addr ffa00000005436e0 by task insmod/81
  ...
   strcmp+0xb0/0xc0
   find_any_unique_sec+0x103/0x190
   load_module+0x5c4/0x8600

sh_name has no defined value for SHT_NULL, so give it one that is in
bounds: the empty string at index 0. The walkers then compare against
"" and, as before, ignore the section, so no valid module is affected.

Fixes: 3c5700aeabd8 ("module: Factor out elf_validity_cache_secstrings")
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 by reading the section-name walkers in kernel/module/main.c after
9a5ff4568932 tightened the string-table types. elf_validity_cache_secstrings()
skips the sh_name bounds check for a SHT_NULL section, but find_any_unique_sec()
still resolves every section's name as secstrings + sh_name, so the skip leaves
one path where an unchecked sh_name is read as a string. Patch 2/2 fixes the
matching SHT_NOBITS gap on __version_ext_names; the two are independent.

Reproducer: build an otherwise valid .ko and add a SHT_NULL section whose
sh_name is past the end of .shstrtab (0x2000 in the run below, aimed at the
redzone after the in-memory module copy). insmod it. Before the change the name
lookup reads out of bounds; after it the SHT_NULL section resolves to "" and, as
before, is ignored, so the module loads with rc=0.

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 tree already contains 9a5ff4568932, so this is
the gap that commit left, not a re-report of it.

One setup detail is not obvious. The payload has to match the kernel's
vermagic to reach load_module()'s name walkers, so the patched kernel is built
with LOCALVERSION pinned to keep the release string byte-identical to the
unpatched one; otherwise insmod fails on the version magic before the buggy
lookup ever runs and the run proves nothing.

 kernel/module/main.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index d0e1e0b..e36bfe4 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2060,9 +2060,18 @@ static int elf_validity_cache_secstrings(struct load_info *info)
 
 	for (i = 0; i < info->hdr->e_shnum; i++) {
 		shdr = &info->sechdrs[i];
-		/* SHT_NULL means sh_name has an undefined value */
-		if (shdr->sh_type == SHT_NULL)
+		/*
+		 * SHT_NULL means sh_name has an undefined value. The section
+		 * name walkers that follow (find_any_unique_sec(),
+		 * module_mark_ro_after_init(), ...) look the name up as
+		 * secstrings + sh_name for every section, so give the undefined
+		 * value a safe in-bounds meaning instead of skipping the check:
+		 * the empty string at index 0.
+		 */
+		if (shdr->sh_type == SHT_NULL) {
+			shdr->sh_name = 0;
 			continue;
+		}
 		if (shdr->sh_name >= strhdr->sh_size) {
 			pr_err("Invalid ELF section name in module (section %u type %u)\n",
 			       i, shdr->sh_type);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 1/2] module: Sanitize the undefined sh_name of SHT_NULL sections

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] module: OOB memory read via SHT_NULL section aliasing
--

commit 536b936ee0704f1857141d93c7d4113160b708c4
Author: Fang Xieyan <fangxy@xiaopeng.com>
module: Sanitize the undefined sh_name of SHT_NULL sections

This commit sets the sh_name of SHT_NULL sections to 0 in order to provide a
safe, in-bounds value for section name walkers. By giving undefined values the
empty string at index 0, valid modules remain unaffected while out-of-bounds
reads are prevented.

> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index d0e1e0bd2ad06..e36bfe4734d59 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2060,9 +2060,18 @@ static int elf_validity_cache_secstrings(struct load_info *info)
>  
>  	for (i = 0; i < info->hdr->e_shnum; i++) {
>  		shdr = &info->sechdrs[i];
> -		/* SHT_NULL means sh_name has an undefined value */
> -		if (shdr->sh_type == SHT_NULL)
> +		/*
> +		 * SHT_NULL means sh_name has an undefined value. The section
> +		 * name walkers that follow (find_any_unique_sec(),
> +		 * module_mark_ro_after_init(), ...) look the name up as
> +		 * secstrings + sh_name for every section, so give the undefined
> +		 * value a safe in-bounds meaning instead of skipping the check:
> +		 * the empty string at index 0.
> +		 */
> +		if (shdr->sh_type == SHT_NULL) {
> +			shdr->sh_name = 0;

[Severity: High]
Does this safely neutralize SHT_NULL sections?

This forcefully sets the name to index 0, assuming index 0 is always
guaranteed to be an empty string. However, earlier in
elf_validity_cache_secstrings(), the string table validation checks the last
byte but fails to ensure that secstrings[0] == '\0':

kernel/module/main.c:elf_validity_cache_secstrings() {
    ...
	if (secstrings[strhdr->sh_size - 1] != '\0') {
		pr_err("ELF Spec violation: section name table isn't null terminated\n");
		return -ENOEXEC;
	}
    ...
}

Because of this, can an attacker craft a string table starting with a critical
section name like .modinfo so that a SHT_NULL section at index i > 0 aliases
to .modinfo?

Since walkers like find_any_unique_sec() do not filter out SHT_NULL sections,
this spoofed name at index 0 could be successfully matched:

kernel/module/main.c:find_any_unique_sec() {
    ...
	for (i = 1; i < info->hdr->e_shnum; i++) {
		if (strcmp(info->secstrings + info->sechdrs[i].sh_name,
			   name) == 0) {
    ...
}

Furthermore, SHT_NULL sections are explicitly exempted from offset validation
in elf_validity_cache_sechdrs():

kernel/module/main.c:elf_validity_cache_sechdrs() {
    ...
		switch (shdr->sh_type) {
		case SHT_NULL:
		case SHT_NOBITS:
			/* No contents, offset/size don't mean anything */
			continue;
    ...
}

Could this allow functions like get_next_modinfo() to dereference the
unvalidated sh_offset of the aliased SHT_NULL section?

kernel/module/main.c:get_next_modinfo() {
    ...
	char *modinfo = (char *)info->hdr + infosec->sh_offset;
    ...
}

Could this lead to an out-of-bounds memory read and a potential kernel panic?

>  			continue;
> +		}
>  		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/20260917190618.94759-1-fangxy@xiaopeng.com?part=1

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

[Fang Xieyan]

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)