Re: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr

Steven Rostedt
2024-04-29T00:29:00Z

On Fri, 26 Apr 2024 15:34:08 +0800
Tze-nan wu <Tze-nan.Wu@mediatek.com> wrote:

> "tracing_event_file" is at the risk of use-after-free due to the race of
> two functions "tracing_open_file_tr" and "synth_event_release".
> Specifically, it could be freed by synth_event_release before
> tracing_open_file_tr has the opportunity to access its members.
>
> It's easy to reproduced by first executing script 'A' and then script 'B'
> in my environment.
>
> Script 'A':
> echo "hello int aaa" > /sys/kernel/tracing/synthetic_events
> while :
> do
> echo 0 > /sys/kernel/tracing/events/synthetic/hello/enable
> done
>
> Script 'B':
> echo > /sys/kernel/tracing/synthetic/events

I think you meant:

echo > /sys/kernel/tracing/synthetic_events

>
> My environment:
> arm64 + generic and swtag based KASAN both enabled + kernel-6.6.18
>
> KASAN report
> ==================================================================
> BUG: KASAN: slab-use-after-free in tracing_open_file_tr
> Read of size 8 at addr 9*ffff********** by task sh/3485
> Pointer tag: [9*], memory tag: [fe]
>
> CPU: * PID: 3485 Comm: sh Tainted: ****************
> Call trace:
> __hwasan_tag_mismatch
> tracing_open_file_tr
> do_dentry_open
> vfs_open
> path_openat
>
> Freed by task 3490:
> slab_free_freelist_hook
> kmem_cache_free
> event_file_put
> remove_event_file_dir
> __trace_remove_event_call
> trace_remove_event_call
> synth_event_release
> dyn_events_release_all
> synth_events_open
>
> page last allocated via order 0, migratetype Unmovable:
> __slab_alloc
> kmem_cache_alloc
> trace_create_new_event
> trace_add_event_call
> register_synth_event
> __create_synth_event
> create_or_delete_synth_event
> trace_parse_run_command
> synth_events_write
> vfs_write

Thanks for reporting this.

>
> Based on the assumption that eventfs_inode will persist throughout the
> execution of the tracing_open_file_tr function,
> we can fix this issue by incrementing the reference count of
> trace_event_file once it is assigned to eventfs_inode->data.
> The reference count will then be decremented during the release phase of
> eventfs_inode.
>
> This ensures that trace_event_file is not prematurely freed while the
> tracing_open_file_tr function is being called.
>
> Fixes: bb32500fb9b7 ("tracing: Have trace_event_file have ref counters")
> Co-developed-by: Cheng-Jui Wang <cheng-jui.wang@mediatek.com>
> Signed-off-by: Cheng-Jui Wang <cheng-jui.wang@mediatek.com>
> Signed-off-by: Tze-nan wu <Tze-nan.Wu@mediatek.com>
> ---
> BTW, I've also attempted to reproduce the same issue in another
> environment (described below).
> It's also reproducible but in a lower rate.
>
> another environment:
> x86 + ubuntu + generic kasan enabled + kernel-6.9-rc4
>
> After applying the patch, KASAN no longer reports any issues when
> following the same reproduction steps in my arm64 environment.
> However, I am concerned about potential side effects that the patch might introduce.
> Additionally, the newly added function "is_entry_event_callback" may not
> be reliable in my opinion,
> as the callback function used in the comparison could change in future.
> Nonetheless, this is the best solution I can come up with now.
>
> Looking for any suggestion or solution, appreciate.

Yeah, I do not think eventfs should be involved in this. It needs to be
protected at a higher level (in the synthetic/dynamic event code).

I'm just coming back from Japan, and I'll need to take a deeper look at
this after I recover from my jetlag.

Thanks,

-- Steve