Message-ID: <70f81863-2972-47da-a7c4-486a0d6062e8@suse.com>
Date: Mon, 15 Sep 2025 14:27:26 +0200
From: Petr Pavlu <petr.pavlu@...e.com>
To: "Daniel v. Kirschten" <danielkirschten@...il.com>
Cc: mcgrof@...nel.org, Daniel Gomez <da.gomez@...nel.org>,
 Sami Tolvanen <samitolvanen@...gle.com>, linux-modules@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] kernel/module: avoid panic when loading corrupt module

On 9/9/25 6:46 PM, Daniel v. Kirschten wrote:
> If the kernel attempts loading a corrupted module where the
> .gnu.linkonce.this_module section is not marked as WRITE,
> the corresponding this_module struct will be remapped read-only
> in the module loading process. This causes a kernel panic later -
> specifically the first time that struct is being written to after the remap.
> (Currently, this happens in complete_formation at kernel/module/main.c:3265,
> when the module state is set to COMING,
> but this doesn't really matter and of course might also change in the future.)
> 
> This panic also causes problems down the line:
> after this panic has occurred, all further attempts
> to add or remove modules will freeze the process attempting to do so.
> I did not investigate this further.
> 
> The kernel's module building toolchain will not produce such module files.
> However, there's only a single bit difference on-disk
> between a correct module file and one which triggers this panic.
> Also, there are modules which aren't produced by the kernel's module toolchain.
> (Yes, we don't necessarily need to fully support such modules,
> but we shouldn't panic when loading them either.)
> 
> Note that from a security point of view, this bug is irrelevant:
> the problematic remap of this_module as readonly
> only happens after all security checks have already passed.
> If an attacker is in the position to insert arbitrary modules into the kernel,
> then it doesn't matter anymore that it's possible to cause a panic too.
> And even in the hypothetical scenario where an attacker
> can trigger this panic but can't insert custom modules,
> the worst that could happen is a DoS
> caused by future module insertion / removal attempts.
> 
> Signed-off-by: Daniel Kirschten <danielkirschten@...il.com>

Nits:

I suggest making the subject of the patch more specific to avoid
potential confusion. For instance:

module: Check that .gnu.linkonce.this_module is writable

I think the description could also be a bit more straightforward and the
text should ideally be reflown to a 75-column boundary (see
Documentation/process/submitting-patches.rst).

> ---
> 
> I hope that the wording is clear enough now about this not being a security bug.
> What do you think?
> 
> In my first submisison of this patch (on 06/06/2024),
> I was told to add this check to userspace kmod too.
> If I find the time, I will do so, but I think that won't help as much
> because there are of course other ways for userspace to load a module,
> such as any alternative insmod implementation (for example busybox).
> 
> Regarding your "next-level university assignment":
> I feel knowing whether the kernel toolchain can or cannot produce such modules
> is a bit beside the point: _if_ such a module is encountered,
> the kernel's going to panic, and it's not going to care where the module came from.
> Also I'm a bit stumped by your wording "university assignment" here.
> Still, I recognize that it would be goot to be certain
> that the official tools don't produce broken modules.
> So, I debugged the module build system a bit and found out the following:
> 
> add_header in scripts/mod/modpost.c:1834-1843 is responsible
> for arranging for the .gnu.linkonce.this_module section to exist:
> The C code this function emits defines the symbol __this_module
> with two attributes: `visibility("default")` and `section(".gnu.linkonce.this_module")`.
> In particular, __this_module is not marked const or anything similar,
> and there definitely is no (supported) way
> for the user to add custom modifiers to this symbol.
> When gcc compiles that file, the resulting section is marked WRITE and ALLOC.
> This seems to be default behaviour of gcc / ld,
> but I couldn't find explicit documentation about this.
> I even tried digging in gcc's source code to find hard proof,
> but as expected gcc turns out to be quite convoluted, so eventually I gave up.
> 
>  kernel/module/main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b26184936..c415c58b9462 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2092,6 +2092,12 @@ static int elf_validity_cache_index_mod(struct load_info *info)
>          return -ENOEXEC;
>      }
>  
> +    if (!(shdr->sh_flags & SHF_WRITE)) {
> +        pr_err("module %s: .gnu.linkonce.this_module must be writable\n",
> +               info->name ?: "(missing .modinfo section or name field)");
> +        return -ENOEXEC;
> +    }
> +
>      if (shdr->sh_size != sizeof(struct module)) {
>          pr_err("module %s: .gnu.linkonce.this_module section size must match the kernel's built struct module size at run time\n",
>                 info->name ?: "(missing .modinfo section or name field)");

This looks ok to me and in line with other checks in
elf_validity_cache_index_mod(), although we should be careful about the
number of ELF sanity checks performed by the module loader.

I think the important part is that the module loader should always at
least get through the signature and blacklist checks without crashing
due to a corrupted ELF file. After that point, the module content is
to be trusted, but we try to error out for most issues that would cause
problems later on. The added check is useful for the latter.

-- 
Thanks,
Petr

Powered by blists - more mailing lists