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
