Hi Kees,
Could you help clarify the handling of __ro_after_init? What do you
think is the best approach when a second attempt fails at setting a
section to RO after a module is already initialized? (please find the
deatils in this pach series or in [1]. Reporting the failure to the
caller seems logical to me but adds some complexity. On the other hand,
logging alone feels insufficient but may be the simplest option. Could
you advice on handling this corner case and if it's relevant to KSPP?
You can find below the conversation. And the v1 PATCH series here [1].
[1]
https://lore.kernel.org/all/cover.1733427536.git.christophe.leroy@xxxxxxxxxx/
Thanks,
Daniel
On 12/10/2024 11:49 AM, Daniel Gomez wrote:
On 12/4/2024 4:14 PM, Petr Pavlu wrote:
On 11/28/24 21:23, Daniel Gomez wrote:
On 11/12/2024 3:35 PM, Petr Pavlu wrote:
On 11/12/24 10:43, Daniel Gomez wrote:
On Mon Nov 11, 2024 at 7:53 PM CET, Christophe Leroy wrote:
Le 09/11/2024 à 23:17, Daniel Gomez a écrit :
On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
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.
Reported-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Closes: https://protect2.fireeye.com/v1/url?k=d3deb284-b2a35ac3-
d3df39cb-74fe485fff30-288375d7d91e4ad9&q=1&e=701066ca-634d-4525-
a77d-1a44451f897a&u=https%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flore.kernel.org%252Fall%252F20230915082126.4187913-1-ruanjinjie%2540huawei.com%252F%26data%3D05%257C02%257Cchristophe.leroy%2540csgroup.eu%257C26b5ca7363e54210439b08dd010c4865%257C8b87af7d86474dc78df45f69a2011bb5%257C0%257C0%257C638667874457200373%257CUnknown%257CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%253D%253D%257C0%257C%257C%257C%26sdata%3DZeJ%252F3%252B2Nx%252FBf%252FWLFEkhxKlDhZk8LNkz0fs%252Fg2xMcOjY%253D%26reserved%3D0
Fixes: d1909c022173 ("module: Don't ignore errors from
set_memory_XX()")
Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
---
kernel/module/main.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 2de4ad7af335..1bf4b0db291b 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct
module *mod)
#endif
ret = module_enable_rodata_ro_after_init(mod);
if (ret)
- goto fail_mutex_unlock;
+ pr_warn("%s: %s() returned %d, ro_after_init data might
still be writable\n",
+ mod->name, __func__, ret);
+
mod_tree_remove_init(mod);
module_arch_freeing_init(mod);
for_class_mod_mem_type(type, init) {
@@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct
module *mod)
return 0;
I think it would make sense to propagate the error. But that would
require changing modprobe.c. What kind of error can we expect
when this
happens?
AFAIK, on powerpc it fails with EINVAL when
- The area is a vmalloc or module area and is a hugepage area
- The area is not vmalloc or io register and MMU is not powerpc
radix MMU
Otherwise it propagates the error from
apply_to_existing_page_range().
IIUC it will return EINVAL when it hits a leaf PTE in upper
directories.
Looking at that path I see we can also fail at __apply_to_page_range()
-> apply_to_p4d_range() and return with -ENOMEM.
My proposal was to do something like the change below in modprobe:
diff --git a/tools/modprobe.c b/tools/modprobe.c
index ec66e6f..8876e27 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -572,6 +572,11 @@ static int insmod_insert(struct kmod_module
*mod, int flags, const char *extra_o
err = 0;
else {
switch (err) {
+ case -EINVAL:
+ ERR("module '%s'inserted: ro_after_init
data might"
+ "still be writable (see dmesg)\n",
+ kmod_module_get_name(mod));
+ break;
case -EEXIST:
ERR("could not insert '%s': Module
already in kernel\n",
kmod_module_get_name(mod));
But I think these error codes may be also be reported in other parts
such as simplify_symbols() so may not be a good idea after all.
It isn't really possible to make a sensible use of the return code from
init_module(), besides some basic check for -EEXIST. The problem is
that
any error code from a module's init function is also propagated as
a result from the syscall.
Maybe we just need to change the default/catch all error message in
modprobe.c and to indicate/include this case:
diff --git a/tools/modprobe.c b/tools/modprobe.c
index ec66e6f..3647d37 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -582,7 +582,8 @@ static int insmod_insert(struct kmod_module
*mod, int flags, const char *extra_o
kmod_module_get_name(mod));
break;
default:
- ERR("could not insert '%s': %s\n",
kmod_module_get_name(mod),
+ ERR("could not insert '%s' or inserted with
error %s, "
+ "(see dmesg)\n",
kmod_module_get_name(mod),
strerror(-err));
break;
}
On other architectures it can be different, I know some
architecture try
to split the pages when they hit hugepages and that can fail.
Is it worth it adding an error code for this case in case we want to
report it back?
I feel that the proposed kernel warning about this situation is
sufficient and the loader should then return 0 to indicate that the
module got loaded. It would be more confusing to return an error but
with the module actually remaining inserted.
A module loaded without having its RO-after-init section changed
properly to RO is still fully functional. In practice, if this final
set_memory_ro() call fails, the system is already in such a state where
the additional warning is the least of the issues?
__ro_after_init is used for kernel self protection. We are loading
"successfully" the module yes, but variables with this attribute are
marked read-only to reduce the attack surface [1]. Since we have
considered this stage already too late to unload the module, IMHO we
should at least indicate that there was an error during the module
initialization and propagate that to the loader, so it can decide the
best action for their particular case. Warning once in the kernel log
system, does not seem sufficient to me.
[1] Documentation/security/self-protection.rst
I'd be careful about introducing this new state where (f)init_module()
returns an error, yet the module actually gets loaded.
Perhaps we just need this new stage? module loaded with permission/
security error?
The init_module() interface has one return value which can originate
from anywhere during the load process, including from the module's own
init function. As mentioned, this means that the userspace cannot
distinguish why the module load failed. It would be needed to have
a specific error code returned only for this case, or to extend the
interface further in some way.
Another concern is consistency of the module loader interface as
a whole. Returning an error from init_module() in this case would mean
that only the specific caller is made aware that the module was loaded
with some issues. A different task that then decides to check the module
state under /sys/module would see it as normally loaded, and similarly a
Maybe we need to change this state too to indicate the problem.
task trying to insert it again would get -EEXIST. That likely would need
changing too.
What I'd like to understand is how reporting this as an error to the
userspace would help in practice. I think the userspace can decide to
report it as a warning and continue, or treat is as a hard problem and
stop the system? I would expect that most tools/systems would opt for
the former, but then this doesn't seem much different to me than when
the kernel produces the warning itself. The second option, with some
stretch, could be implemented with panic_on_warn=1.
Ideally, we would reverse the module initialization when encountering
this error [1]. However, since it's not feasible to undo it correctly at
this stage, reporting the error back to the caller allows them to assess
and decide whether to accept the risk.
[1] https://lore.kernel.org/all/Zuv0nmFblHUwuT8v@xxxxxxxxxxxxxxxxxxxxxx/
Do you envision that the userspace would handle this problem differently
and it is worth adding the complexity?
What complexity do you mean?
A module driver has ro_after_init for the purpose of protecting the
kernel from attack [2]. If we ignore it by warning once, the caller will
not be aware of such risk (unless the caller it's parsing the kernel log).
[2] https://lore.kernel.org/all/1455748879-21872-1-git-send-email-
keescook@xxxxxxxxxxxx/
Another option could be adding a kconfig to decide to report or not
which I would strongly suggest to report by default.
As a side node, I've noticed that the module loader could mark also
static_call sections as ro_after_init. I'll post patches for that.
Additionally, both __jump_table and static_call sections could be
possibly turned RO earlier, after prepare_coming_module() ->
blocking_notifier_call_chain_robust() -> ... ->
jump_label_add_module()/static_call_add_module().
