On 2025/5/27 21:41, Steven Rostedt wrote:
On Mon, 26 May 2025 09:33:37 +0800
yebin <ye...@huaweicloud.com> wrote:
On 2025/5/24 1:54, Steven Rostedt wrote:
On Fri, 23 May 2025 16:39:44 +0800
Ye Bin <ye...@huaweicloud.com> wrote:
Above issue may happens as follow:
(1) Add kprobe trace point;
(2) insmod test.ko;
(3) Trigger ftrace disabled;
This is the bug. How was ftrace_disabled triggered? That should never
happen. Was test.ko buggy?
Yes. The following warning is reported during concurrent registration
between register_kprobe() and live patch, causing ftrace_disabled.
WARNING: CPU: 56 PID: 2769 at kernel/trace/ftrace.c:2612
ftrace_modify_all_code+0x116/0x140
OK, so it is a buggy module.
(4) rmmod test.ko;
(5) cat /proc/kallsyms; --> Will trigger UAF as test.ko already removed;
ftrace_mod_get_kallsym()
...
strscpy(module_name, mod_map->mod->name, MODULE_NAME_LEN);
...
As ftrace_release_mod() judge 'ftrace_disabled' is true will return, and
'mod_map' will remaining in ftrace_mod_maps. 'mod_map' has no chance to
release. Therefore, this also causes residual resources to accumulate.
To solve above issue, unconditionally clean up'mod_map'.
Fixes: aba4b5c22cba ("ftrace: Save module init functions kallsyms symbols for
tracing")
This is *not* a fix. ftrace_disabled gets set when a bug is triggered. If
this prevents ftrace_disabled from getting set, then it would be a fix. But
if something else happens when ftrace_disabled is set, it just fixes a
symptom and not the bug itself.
There are multiple causes for triggering ftrace_disabled. I agree that
Yes, just like there's multiple causes for BUG_ON() ;-)
The ftrace_disable is used to help keep the system from being totally
corrupted. When it triggers, the best thing to do is a reboot.
aba4b5c22cba is not faulty. However, the incorporation of this patch
will cause problems due to triggering ftrace_disabled. The generation of
ftrace_disabled is beyond our control. This is related to the user. What
we can do is even if there are no additional derivative problems.
Well, when a user inserts a module, then they become a kernel developer too ;-)
Signed-off-by: Ye Bin <yebi...@huawei.com>
---
kernel/trace/ftrace.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a3d4dfad0cbc..ff5d9d73a4a7 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -7438,9 +7438,6 @@ void ftrace_release_mod(struct module *mod)
mutex_lock(&ftrace_lock);
- if (ftrace_disabled)
- goto out_unlock;
-
Here you delete the check, and the next patch you have:
+ if (ftrace_disabled || (mod && !mod->num_ftrace_callsites)) {
+ mutex_unlock(&ftrace_lock);
+ return;
+ }
+
The second patch I added judgment when initializing 'mod_map' in
ftrace_free_mem(). The first patch removes the judgment when
ftrace_release_mod() releases'mod_map'. The logic modified by the two
patches is isolated.
Actually I think both patches are buggy.
When ftrace_disabled is set, we don't know the state of the code and we do
not want to do *any* more text modification. That's what ftrace_disable
means. Something went wrong with text modification and any more changes can
cause a bigger problem.
This problem can be solved by releasing the 'mod_map' resource when the
module is unloaded. Freeing up these resources is just an address that
cannot be translated into symbols, and there are no worse consequences.
We don't add "exceptions".
If you are worried about unloading modules when ftrace_disable is set, what
is a much safer solution is to up the module count of all modules that have
any ftrace callsites active, and prevent those modules from being removed.
I don't think it's necessary to introduce logic to restrict module
unloading here, which doesn't bring benefits but increases the cost of
interpretation for maintainers.
Again, the only solution to a ftrace_disable being set is a full reboot.
We can't ask users to know such specialized details of the
implementation, which are unclear even to developers unfamiliar with the
ftrace module. Users can accept planned reboot system recovery, but
should not accept casual operations and the system crashes.All we can do
is do a good job of protection, give users more tolerance.Perhaps a
system that is dead but won't lie down is also a very undesirable
situation.However, ftrace is used to collect information and locate
faults. Even if it does not work, it does not affect services.In the
production environment, the most afraid of using ftrace suddenly crashes
the system.Therefore, the robustness of the tool itself is very important.
-- Steve
I reworked the two patches, and the changes to the existing process
should be minimal. I don't know if I can get your approval. If you
agree, I'll post another V3 version.
PATCH[1/2]:
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 51580e54677f..b3436d86e470 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -7438,9 +7438,10 @@ void ftrace_release_mod(struct module *mod)
mutex_lock(&ftrace_lock);
- if (ftrace_disabled)
- goto out_unlock;
-
+ /*
+ * To avoid the UAF problem after the module is unloaded, the
+ * 'mod_map' resource needs to be released unconditionally.
+ */
list_for_each_entry_safe(mod_map, n, &ftrace_mod_maps, list) {
if (mod_map->mod == mod) {
list_del_rcu(&mod_map->list);
@@ -7451,6 +7452,9 @@ void ftrace_release_mod(struct module *mod)
}
}
+ if (ftrace_disabled)
+ goto out_unlock;
+
/*
* Each module has its own ftrace_pages, remove
* them from the list.
PATCH[2/2]:
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a3d4dfad0cbc..51580e54677f 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -7629,6 +7629,9 @@ allocate_ftrace_mod_map(struct module *mod,
{
struct ftrace_mod_map *mod_map;
+ if (ftrace_disabled)
+ return NULL;
+
mod_map = kmalloc(sizeof(*mod_map), GFP_KERNEL);
if (!mod_map)
return NULL;
