linux-kernel - Re: [PATCH 1/2] ftrace: fix UAF when lookup kallsym after ftrace disabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250528101318.5ee12329@gandalf.local.home>
Date: Wed, 28 May 2025 10:13:18 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: yebin <yebin@...weicloud.com>
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 Wed, 28 May 2025 21:22:37 +0800
yebin <yebin@...weicloud.com> wrote:

> 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.

OK, I'm fine with releasing the mod_map resource without doing the text
modifications.

> 
> > 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.

Preventing ftrace from crashing the system is the reason ftrace_disabled is
set and stops it from doing any more damage.

If you are worried about users not knowing that a reboot is necessary, we
could always add the "Dazed and confused, but trying to continue" message
that could also recommend a reboot.

> > -- 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.

Yes, this is more appropriate.

Thanks,

-- Steve

> 
> 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;