kernel/module/internal.h | 3 ++- kernel/module/main.c | 13 +++++++------ kernel/module/strict_rwx.c | 16 ++++++++++++---- 3 files changed, 21 insertions(+), 11 deletions(-)
This series reworks module loading to avoid leaving the module in a
stale state when protecting ro_after_init section fails.
Once module init has succeded it is too late to cancel loading.
If setting ro_after_init data section to read-only fails, all we can
do is to inform the user through a warning. This is what patch 2 does.
Then patch 3 tries to go a bit further by testing the ability to write
protect ro-after-init section prior to initialising the module.
Changes between RFC and v1:
- Patch 2: Fixed comment from Petr about __func__
- Patch 3: Expanded the commit message based on feedback from RFC series
Christophe Leroy (3):
module: Split module_enable_rodata_ro()
module: Don't fail module loading when setting ro_after_init section
RO failed
module: pre-test setting ro_after_init data read-only
kernel/module/internal.h | 3 ++-
kernel/module/main.c | 13 +++++++------
kernel/module/strict_rwx.c | 16 ++++++++++++----
3 files changed, 21 insertions(+), 11 deletions(-)
--
2.47.0
On 12/5/24 20:46, Christophe Leroy wrote: > This series reworks module loading to avoid leaving the module in a > stale state when protecting ro_after_init section fails. > > Once module init has succeded it is too late to cancel loading. > If setting ro_after_init data section to read-only fails, all we can > do is to inform the user through a warning. This is what patch 2 does. > > Then patch 3 tries to go a bit further by testing the ability to write > protect ro-after-init section prior to initialising the module. I've been holding off on applying this series to modules-next because there was still some discussion on the previous RFC version [1], and I wanted to give people more time to potentially comment. Mike Rapoport also recently posted a series with a patch [2] that proposes restoring of large pages after fragmentation. Should the last patch here be then dropped? [1] https://lore.kernel.org/linux-modules/737f952790c96a09ad5e51689918b97ef9b29174.1731148254.git.christophe.leroy@csgroup.eu/ [2] https://lore.kernel.org/linux-modules/20241227072825.1288491-4-rppt@kernel.org/ -- Thanks, Petr
On Fri, Jan 03, 2025 at 05:13:32PM +0100, Petr Pavlu wrote: > On 12/5/24 20:46, Christophe Leroy wrote: > > This series reworks module loading to avoid leaving the module in a > > stale state when protecting ro_after_init section fails. > > > > Once module init has succeded it is too late to cancel loading. Is there at least a big WARN about the ro failing? That should let more sensitive system owners react to the situation if it looks like an active attack on memory protections. (And maybe we should set a TAINT flag, but perhaps this is too specific a failure mode for that?) Also, why is it too late to cancel? Can we set the module to the "Unloading" state to stop any dependent modules from loading on top of it, and then request it unload? -- Kees Cook
On Mon, Jan 06, 2025 at 04:13:29PM -0800, Kees Cook wrote: > On Fri, Jan 03, 2025 at 05:13:32PM +0100, Petr Pavlu wrote: > > On 12/5/24 20:46, Christophe Leroy wrote: > > > This series reworks module loading to avoid leaving the module in a > > > stale state when protecting ro_after_init section fails. > > > > > > Once module init has succeded it is too late to cancel loading. > > Is there at least a big WARN about the ro failing? That should let more > sensitive system owners react to the situation if it looks like an > active attack on memory protections. > > (And maybe we should set a TAINT flag, but perhaps this is too specific > a failure mode for that?) I don't see a taint flag too far fetched in value. I think its a sensible compromise, and may be useful for other future set_memory_*() failures. Luis
On Mon, Jan 06, 2025 at 04:13:29PM -0800, Kees Cook wrote: > On Fri, Jan 03, 2025 at 05:13:32PM +0100, Petr Pavlu wrote: > > On 12/5/24 20:46, Christophe Leroy wrote: > > > This series reworks module loading to avoid leaving the module in a > > > stale state when protecting ro_after_init section fails. > > > > > > Once module init has succeded it is too late to cancel loading. > > Is there at least a big WARN about the ro failing? That should let more > sensitive system owners react to the situation if it looks like an > active attack on memory protections. Yes, there is. But I think only the first time a module fails. IIUC, any subsequent modules failing will not warn anything, is that right? However, I think this should suffice to know a system is vulnerable but will not know the full list of the actual vulnerable modules. > > (And maybe we should set a TAINT flag, but perhaps this is too specific > a failure mode for that?) > > Also, why is it too late to cancel? Can we set the module to the > "Unloading" state to stop any dependent modules from loading on top of > it, and then request it unload? I think Luis summarized it here [1]. Quoting him from that thread: Sadly there are a few issues with trying to get to call mod->exit(): - We should try try_stop_module() and that can fail - source_list may not be empty and that would block removal - mod->exit may not exist https://lore.kernel.org/all/Zuv0nmFblHUwuT8v@bombadil.infradead.org/ Module loading goes from UNFORMED to LIVE during the initialization. And once it's LIVE we do the ro_after_init memory protection. I'm not sure if an intermediate stage can be added so ro_after_init is performed and module is unloaded when this fails? I guess init does not necessary mean LIVE. Daniel > > -- > Kees Cook
Le 03/01/2025 à 17:13, Petr Pavlu a écrit : > On 12/5/24 20:46, Christophe Leroy wrote: >> This series reworks module loading to avoid leaving the module in a >> stale state when protecting ro_after_init section fails. >> >> Once module init has succeded it is too late to cancel loading. >> If setting ro_after_init data section to read-only fails, all we can >> do is to inform the user through a warning. This is what patch 2 does. >> >> Then patch 3 tries to go a bit further by testing the ability to write >> protect ro-after-init section prior to initialising the module. > > I've been holding off on applying this series to modules-next because > there was still some discussion on the previous RFC version [1], and > I wanted to give people more time to potentially comment. > > Mike Rapoport also recently posted a series with a patch [2] that > proposes restoring of large pages after fragmentation. Should the last > patch here be then dropped? Indeed, if the large pages are restored when bringing back the ro_after_init to RW, it defeats the purpose of patch 3. So I agree, let's first apply patches 1 and 2 in order to fix the actual bug then see how we can improve as a second step. > > [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-modules%2F737f952790c96a09ad5e51689918b97ef9b29174.1731148254.git.christophe.leroy%40csgroup.eu%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Ce1338eec4ee742a40b6208dd2c1192dc%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638715176198708012%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=B7qL4g0NUqBqdREndab5kywoOu2wsNYej6hqnIH10tk%3D&reserved=0 > [2] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-modules%2F20241227072825.1288491-4-rppt%40kernel.org%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Ce1338eec4ee742a40b6208dd2c1192dc%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638715176198723794%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=eMxPsu43ByOjr7ny9Xg81ylWS6853dTU5MmU3J9e2hc%3D&reserved=0 >
On 1/4/25 08:39, Christophe Leroy wrote: > Le 03/01/2025 à 17:13, Petr Pavlu a écrit : >> On 12/5/24 20:46, Christophe Leroy wrote: >>> This series reworks module loading to avoid leaving the module in a >>> stale state when protecting ro_after_init section fails. >>> >>> Once module init has succeded it is too late to cancel loading. >>> If setting ro_after_init data section to read-only fails, all we can >>> do is to inform the user through a warning. This is what patch 2 does. >>> >>> Then patch 3 tries to go a bit further by testing the ability to write >>> protect ro-after-init section prior to initialising the module. >> >> I've been holding off on applying this series to modules-next because >> there was still some discussion on the previous RFC version [1], and >> I wanted to give people more time to potentially comment. >> >> Mike Rapoport also recently posted a series with a patch [2] that >> proposes restoring of large pages after fragmentation. Should the last >> patch here be then dropped? > > Indeed, if the large pages are restored when bringing back the > ro_after_init to RW, it defeats the purpose of patch 3. > > So I agree, let's first apply patches 1 and 2 in order to fix the actual > bug then see how we can improve as a second step. I've now queued the first two patches on modules-next. -- Thanks, Petr
On Thu, Dec 05, 2024 at 08:46:14PM +0100, Christophe Leroy wrote: > This series reworks module loading to avoid leaving the module in a > stale state when protecting ro_after_init section fails. > > Once module init has succeded it is too late to cancel loading. > If setting ro_after_init data section to read-only fails, all we can > do is to inform the user through a warning. This is what patch 2 does. > > Then patch 3 tries to go a bit further by testing the ability to write > protect ro-after-init section prior to initialising the module. Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> Luis
© 2016 - 2025 Red Hat, Inc.