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

[Petr Pavlu <petr.pavlu@suse.com>]

On 9/5/26 1:08 PM, liuc63 wrote:
> From: Liu Chao <liuc63@xiaopeng.com>
> 
> elf_validity_cache_secstrings() checks that every section name offset is
> inside the section name table, but skips headers of type SHT_NULL, since
> ELF leaves their field values undefined.
> 
> However, the section lookup helpers do not check sh_type. 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) {
> 
> find_any_sec() and module_enforce_rwx_sections() read sh_name the same
> way, the latter from index 0. find_sec() is only shielded by its
> SHF_ALLOC test, which a crafted module can satisfy.
> 
> ELF also permits SHT_NULL entries above index 0. A module that carries
> one with a large sh_name passes validation and then reads out of bounds:
> 
>   BUG: unable to handle page fault for address: ffffc9004052f988
>   Oops: 0000 [#1] SMP KASAN PTI
>   CPU: 0 UID: 0 PID: 261 Comm: insmod Not tainted 6.18.0-rc7 #2
>   RIP: 0010:strcmp+0xc/0x40
>   Call Trace:
>    ? find_any_unique_sec+0x89/0xf0
>    load_module+0x516/0x3d30
>    init_module_from_file+0xf5/0x180
>    __x64_sys_finit_module+0x91/0x100
> 
> The read happens in elf_validity_cache_copy(), before the blacklist check
> in early_mod_check().
> 
> Bounds check sh_name for every section header. The name table is already
> known to be non-empty, so the sh_name of 0 that ELF requires at index 0
> still passes, and a name offset that points outside the name table is
> malformed regardless of section type. Neither an Ubuntu 20.04 x86_64
> build (GCC 9.4, 6025 modules) nor an aarch64 vendor build (GCC 13.2, 127
> modules) has a header with an out-of-bounds sh_name, or any SHT_NULL
> header above index 0.
> 
> Link: https://lore.kernel.org/linux-modules/20260708013301.C4D4A1F000E9@smtp.kernel.org/
> Cc: stable@vger.kernel.org
> Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
> ---
> The same report lists two more SHT_NULL gaps, both from
> elf_validity_cache_sechdrs skipping offset and size validation:
> move_module will memcpy such a section, and elf_validity_cache_index_mod
> rejects SHT_NOBITS but not SHT_NULL. Sending this one on its own first,
> happy to respin all three as a series if you would rather have them
> together.
> 
> The Oops line says KASAN, but the access faults rather than producing a
> KASAN report, because lib/string.o is built with KASAN_SANITIZE disabled.
> 
> No Fixes tag. 3c5700aeabd8 ("module: Factor out
> elf_validity_cache_secstrings") is where the exemption was added, but the
> problem predates it: before that commit sh_name was only checked for
> SHF_ALLOC sections and the ".modinfo" strcmp() ran ahead of the check, so
> find_any_sec() could already walk off the end. Same conclusion as
> 9a5ff4568932 ("module: validate string table section types"), which went
> in with Cc: stable and no Fixes tag. checkpatch complains about that
> combination.
> 
> The scan behind that last paragraph applies exactly the predicate this
> patch introduces, sh_name >= strhdr->sh_size, to every section header
> from index 0. On the x86_64 host it covered 17546 modules across three
> kernel trees (5.15.0-181, 5.15.0-190, 5.4.0-182), all built by GCC 9.4.0;
> those trees are the same toolchain recompiling much the same driver set,
> so the changelog quotes one tree rather than the total. The aarch64 side
> is a 5.10 vendor kernel built with Arm GNU Toolchain 13.2. Both are GNU
> toolchains - no clang-built module set was available to me.
> 
> Reproducer, needs root and an unsigned module on a kernel without
> CONFIG_MODULE_SIG_FORCE. Build any minimal module, then append a
> SHT_NULL section header with an out-of-bounds sh_name:
> 
>   python3 - <<'EOF'
>   import struct
>   SHDR = 0x40
>   data = bytearray(open('dummy.ko', 'rb').read())
>   shoff = struct.unpack_from('<Q', data, 0x28)[0]
>   shnum = struct.unpack_from('<H', data, 0x3c)[0]
>   old = bytes(data[shoff:shoff + shnum * SHDR])
>   evil = bytearray(SHDR)
>   struct.pack_into('<I', evil, 0, 0x40000000)
>   data += b'\0' * ((8 - len(data) % 8) % 8)
>   new_shoff = len(data)
>   data += old + bytes(evil)
>   struct.pack_into('<Q', data, 0x28, new_shoff)
>   struct.pack_into('<H', data, 0x3c, shnum + 1)
>   open('evil.ko', 'wb').write(data)
>   EOF
> 
>   insmod evil.ko
> 
>  kernel/module/main.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index d0e1e0bd2ad0..7d5943cd6543 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2060,9 +2060,6 @@ 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)
> -			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);

The ELF specification [1] says:

| SHT_NULL
|
|     This value marks the section header as inactive; it does not have an
|     associated section. Other members of the section header have
|     undefined values.

The patch may cause the module loader to read undefined values if the
section has type SHT_NULL.

I think the module loader should handle SHT_NULL properly by skipping
such sections during all processing. Alternatively, it could reject any
module with an SHT_NULL section other than the one at index 0.

[1] https://gabi.xinuos.com/v42/elf/03-sheader.html#section-type

-- 
Thanks,
Petr