Message-ID: <Zn7x_9BpJLwYSORn@bombadil.infradead.org>
Date: Fri, 28 Jun 2024 10:25:19 -0700
From: Luis Chamberlain <mcgrof@...nel.org>
To: Daniel von Kirschten <danielkirschten@...il.com>
Cc: linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6.10.0-rc2] kernel/module: avoid panic on loading broken
 module

On Fri, Jun 21, 2024 at 04:05:27PM +0200, Daniel von Kirschten wrote:
> Am 18.06.2024 um 21:58 schrieb Luis Chamberlain:
> > On Thu, Jun 06, 2024 at 03:31:49PM +0200, Daniel v. Kirschten wrote:
> > > If a module is being loaded, and the .gnu.linkonce.this_module section
> > > in the module's ELF file does not have the WRITE flag, the kernel will
> > > map the finished module struct of that module as read-only.
> > > This causes a kernel panic when the struct is written to the first time
> > > after it has been marked read-only. Currently this happens in
> > > complete_formation in kernel/module/main.c:2765 when the module's state is
> > > set to MODULE_STATE_COMING, just after setting up the memory protections.
> > 
> > How did you find this issue?
> 
> In a university course I got the assignment to manually craft a loadable .ko
> file, given only a regular object file, without using Kbuild. During testing
> my module files, most of them were simply (correctly) rejected by the kernel
> with an appropriate error message, but at some point I ran into this exact
> kernel panic, and investigated it to understand why my module file was
> invalid.

OK, then the commit log should describe that this doesn't fix any known
real world issue, but rather a custom crafted module without the regular
module build system.

> > > Down the line, this seems to lead to unpredictable freezes when trying to
> > > load other modules - I guess this is due to some structures not being
> > > cleaned up properly, but I didn't investigate this further.
> > > 
> > > A check already exists which verifies that .gnu.linkonce.this_module
> > > is ALLOC. This patch simply adds an analogous check for WRITE.
> > 
> > Can you check to ensure our modules generated have a respective check to
> > ensure this check exists at build time? That would proactively inform
> > userspace when a built module is not built correctly, and the tool
> > responsible can be identified.
> 
> See above - I don't think it's possible to create such a broken module file
> with any of "official" tools.

That should be clearly stated on the commit log.

> I haven't looked too deeply into how Kbuild
> actually builds modules, but as far as I know, the user doesn't even come
> into contact with this_module w

Consider that a next level university assignment and is more useful to the world
than this debug message. Because above you suggest "I don't think", go
out and now be sure.

> hen using the regular toolchain, because
> Kbuild is responsible for creating the .this_module section. And Kbuild of
> course creates it with the correct flags. So if I understand correctly,

...

> this
> problem can only occur when the module was built by some external tooling
> (or manually, in my case).

Who would create custom modules without the Linux kernel module build
system, and what uses does that provide? It seems you are proving why
this would be terribly silly thing to do.

Now, the *value* your change has is it can prevent a crash in case of a
corrupted module, which *can* occur, consider an odd filesystem
live corruption, at least this would be caught at module load attempt
and not crash. That's worth committing for this reason but your commit
log really needs much more clarity. Why? Because stupid bots want to
assign stupid CVEs for anything that seems like a security issue and
this could escalate to such type of things. Providing clarity helps
system integrators decide if they want to backport this sort of patch.
Providing clarify on the chances of this happening and how we think it
can happen helps a lot.

If you want to be more proactive, try to enhance userspace kmod modprobe
so that this is also verified.

  Luis

Powered by blists - more mailing lists