[PATCH v2] kernel/module: avoid panic when loading corrupt module

["Daniel v. Kirschten" <danielkirschten@gmail.com>]

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@gmail.com>
---

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)");
-- 
2.39.5