Message-ID: <68370E1D.3070802@huaweicloud.com>
Date: Wed, 28 May 2025 21:22:37 +0800
From: yebin <yebin@...weicloud.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: mhiramat@...nel.org, mathieu.desnoyers@...icios.com,
 mark.rutland@....com, linux-trace-kernel@...r.kernel.org,
 linux-kernel@...r.kernel.org, yebin10@...wei.com
Subject: Re: [PATCH 1/2] ftrace: fix UAF when lookup kallsym after ftrace
 disabled



On 2025/5/27 21:41, Steven Rostedt wrote:
> On Mon, 26 May 2025 09:33:37 +0800
> yebin <yebin@...weicloud.com> wrote:
>
>> On 2025/5/24 1:54, Steven Rostedt wrote:
>>> On Fri, 23 May 2025 16:39:44 +0800
>>> Ye Bin <yebin@...weicloud.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 <yebin10@...wei.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;

Powered by blists - more mailing lists