LinuxLists.cc - [PATCH] tracing: Fix uaf issue when open the hist or hist

2023-12-12 11:52:33

Subject: [PATCH] tracing: Fix uaf issue when open the hist or hist_debug file

2023-12-12 16:36:18

Subject: Re: [PATCH] tracing: Fix uaf issue when open the hist or hist_debug file

On Tue, 12 Dec 2023 19:33:17 +0800
Zheng Yejian <[email protected]> wrote:

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 1abc07fba1b9..00447ea7dabd 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -5623,10 +5623,12 @@ static int event_hist_open(struct inode *inode, struct file *file)
> {
> int ret;
>
> - ret = security_locked_down(LOCKDOWN_TRACEFS);
> + ret = tracing_open_file_tr(inode, file);
> if (ret)
> return ret;
>
> + /* Clear private_data to avoid warning in single_open */
> + file->private_data = NULL;
> return single_open(file, hist_show, file);
> }
>
> @@ -5634,7 +5636,7 @@ const struct file_operations event_hist_fops = {
> .open = event_hist_open,
> .read = seq_read,
> .llseek = seq_lseek,
> - .release = single_release,
> + .release = tracing_release_file_tr,

single_release() still needs to be called. This can't simply be replaced
with tracing_release_file_tr().

> };
>
> #ifdef CONFIG_HIST_TRIGGERS_DEBUG
> @@ -5900,10 +5902,12 @@ static int event_hist_debug_open(struct inode *inode, struct file *file)
> {
> int ret;
>
> - ret = security_locked_down(LOCKDOWN_TRACEFS);
> + ret = tracing_open_file_tr(inode, file);
> if (ret)
> return ret;
>
> + /* Clear private_data to avoid warning in single_open */
> + file->private_data = NULL;
> return single_open(file, hist_debug_show, file);
> }
>
> @@ -5911,7 +5915,7 @@ const struct file_operations event_hist_debug_fops = {
> .open = event_hist_debug_open,
> .read = seq_read,
> .llseek = seq_lseek,
> - .release = single_release,
> + .release = tracing_release_file_tr,

Same here. This just causes a leak of the single resources.

What needs to be done is to add a:

tracing_single_release_file_tr()

That does both:

int tracing_single_release_file_tr(struct inode *inode, struct file *filp)
{
struct trace_event_file *file = inode->i_private;

trace_array_put(file->tr);
event_file_put(file);

return single_release(inode, filp);
}

-- Steve

> };
> #endif
>

2023-12-13 01:51:18

by Zheng Yejian

[permalink] [raw]

Subject: [PATCH v2] tracing: Fix uaf issue when open the hist or hist_debug file

KASAN report following issue. The root cause is when opening 'hist'
file of an instance and accessing 'trace_event_file' in hist_show(),
but 'trace_event_file' has been freed due to the instance being removed.
'hist_debug' file has the same problem. To fix it, call
tracing_{open,release}_file_tr() in file_operations callback to have
the ref count and avoid 'trace_event_file' being freed.

BUG: KASAN: slab-use-after-free in hist_show+0x11e0/0x1278
Read of size 8 at addr ffff242541e336b8 by task head/190

CPU: 4 PID: 190 Comm: head Not tainted 6.7.0-rc5-g26aff849438c #133
Hardware name: linux,dummy-virt (DT)
Call trace:
dump_backtrace+0x98/0xf8
show_stack+0x1c/0x30
dump_stack_lvl+0x44/0x58
print_report+0xf0/0x5a0
kasan_report+0x80/0xc0
__asan_report_load8_noabort+0x1c/0x28
hist_show+0x11e0/0x1278
seq_read_iter+0x344/0xd78
seq_read+0x128/0x1c0
vfs_read+0x198/0x6c8
ksys_read+0xf4/0x1e0
__arm64_sys_read+0x70/0xa8
invoke_syscall+0x70/0x260
el0_svc_common.constprop.0+0xb0/0x280
do_el0_svc+0x44/0x60
el0_svc+0x34/0x68
el0t_64_sync_handler+0xb8/0xc0
el0t_64_sync+0x168/0x170

Allocated by task 188:
kasan_save_stack+0x28/0x50
kasan_set_track+0x28/0x38
kasan_save_alloc_info+0x20/0x30
__kasan_slab_alloc+0x6c/0x80
kmem_cache_alloc+0x15c/0x4a8
trace_create_new_event+0x84/0x348
__trace_add_new_event+0x18/0x88
event_trace_add_tracer+0xc4/0x1a0
trace_array_create_dir+0x6c/0x100
trace_array_create+0x2e8/0x568
instance_mkdir+0x48/0x80
tracefs_syscall_mkdir+0x90/0xe8
vfs_mkdir+0x3c4/0x610
do_mkdirat+0x144/0x200
__arm64_sys_mkdirat+0x8c/0xc0
invoke_syscall+0x70/0x260
el0_svc_common.constprop.0+0xb0/0x280
do_el0_svc+0x44/0x60
el0_svc+0x34/0x68
el0t_64_sync_handler+0xb8/0xc0
el0t_64_sync+0x168/0x170

Freed by task 191:
kasan_save_stack+0x28/0x50
kasan_set_track+0x28/0x38
kasan_save_free_info+0x34/0x58
__kasan_slab_free+0xe4/0x158
kmem_cache_free+0x19c/0x508
event_file_put+0xa0/0x120
remove_event_file_dir+0x180/0x320
event_trace_del_tracer+0xb0/0x180
__remove_instance+0x224/0x508
instance_rmdir+0x44/0x78
tracefs_syscall_rmdir+0xbc/0x140
vfs_rmdir+0x1cc/0x4c8
do_rmdir+0x220/0x2b8
__arm64_sys_unlinkat+0xc0/0x100
invoke_syscall+0x70/0x260
el0_svc_common.constprop.0+0xb0/0x280
do_el0_svc+0x44/0x60
el0_svc+0x34/0x68
el0t_64_sync_handler+0xb8/0xc0
el0t_64_sync+0x168/0x170

Suggested-by: Steven Rostedt <[email protected]>
Signed-off-by: Zheng Yejian <[email protected]>
---
kernel/trace/trace_events_hist.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)

Steve, thanks for your review!

v2:
- Introduce tracing_single_release_file_tr() to add the missing call for
single_release() as suggested by Steve;
Link: https://lore.kernel.org/all/[email protected]/
- Slightly modify the commit message and comments.

v1:
Link: https://lore.kernel.org/all/[email protected]/

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 1abc07fba1b9..5296a08c0641 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -5619,14 +5619,22 @@ static int hist_show(struct seq_file *m, void *v)
return ret;
}

+static int tracing_single_release_file_tr(struct inode *inode, struct file *filp)
+{
+ tracing_release_file_tr(inode, filp);
+ return single_release(inode, filp);
+}
+
static int event_hist_open(struct inode *inode, struct file *file)
{
int ret;

- ret = security_locked_down(LOCKDOWN_TRACEFS);
+ ret = tracing_open_file_tr(inode, file);
if (ret)
return ret;

+ /* Clear private_data to avoid warning in single_open() */
+ file->private_data = NULL;
return single_open(file, hist_show, file);
}

@@ -5634,7 +5642,7 @@ const struct file_operations event_hist_fops = {
.open = event_hist_open,
.read = seq_read,
.llseek = seq_lseek,
- .release = single_release,
+ .release = tracing_single_release_file_tr,
};

#ifdef CONFIG_HIST_TRIGGERS_DEBUG
@@ -5900,10 +5908,12 @@ static int event_hist_debug_open(struct inode *inode, struct file *file)
{
int ret;

- ret = security_locked_down(LOCKDOWN_TRACEFS);
+ ret = tracing_open_file_tr(inode, file);
if (ret)
return ret;

+ /* Clear private_data to avoid warning in single_open() */
+ file->private_data = NULL;
return single_open(file, hist_debug_show, file);
}

@@ -5911,7 +5921,7 @@ const struct file_operations event_hist_debug_fops = {
.open = event_hist_debug_open,
.read = seq_read,
.llseek = seq_lseek,
- .release = single_release,
+ .release = tracing_single_release_file_tr,
};
#endif

--
2.25.1

2023-12-13 13:01:17

by Steven Rostedt

[permalink] [raw]

Subject: Re: [PATCH v2] tracing: Fix uaf issue when open the hist or hist_debug file

On Wed, 13 Dec 2023 09:51:38 +0800
Zheng Yejian <[email protected]> wrote:

> ---
> kernel/trace/trace_events_hist.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> Steve, thanks for your review!
>
> v2:
> - Introduce tracing_single_release_file_tr() to add the missing call for
> single_release() as suggested by Steve;
> Link: https://lore.kernel.org/all/[email protected]/
> - Slightly modify the commit message and comments.
>
> v1:
> Link: https://lore.kernel.org/all/[email protected]/
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 1abc07fba1b9..5296a08c0641 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -5619,14 +5619,22 @@ static int hist_show(struct seq_file *m, void *v)
> return ret;
> }
>
> +static int tracing_single_release_file_tr(struct inode *inode, struct file *filp)
> +{
> + tracing_release_file_tr(inode, filp);
> + return single_release(inode, filp);
> +}
> +

Hi Zheng,

Could you put the tracing_single_release_file_tr() into trace.c as a non
static function. Put the protocol in kernel/trace/trace.h.

Even though it's only used in trace_events_hist.c, I rather not hide it
there. If this issue appears for another file, I would look in trace.c and
trace.h for available functions to use.

Thanks,

-- Steve

2023-12-14 01:22:09

by Zheng Yejian

[permalink] [raw]

Subject: [PATCH v3] tracing: Fix uaf issue when open the hist or hist_debug file

KASAN report following issue. The root cause is when opening 'hist'
file of an instance and accessing 'trace_event_file' in hist_show(),
but 'trace_event_file' has been freed due to the instance being removed.
'hist_debug' file has the same problem. To fix it, call
tracing_{open,release}_file_tr() in file_operations callback to have
the ref count and avoid 'trace_event_file' being freed.

BUG: KASAN: slab-use-after-free in hist_show+0x11e0/0x1278
Read of size 8 at addr ffff242541e336b8 by task head/190

CPU: 4 PID: 190 Comm: head Not tainted 6.7.0-rc5-g26aff849438c #133
Hardware name: linux,dummy-virt (DT)
Call trace:
dump_backtrace+0x98/0xf8
show_stack+0x1c/0x30
dump_stack_lvl+0x44/0x58
print_report+0xf0/0x5a0
kasan_report+0x80/0xc0
__asan_report_load8_noabort+0x1c/0x28
hist_show+0x11e0/0x1278
seq_read_iter+0x344/0xd78
seq_read+0x128/0x1c0
vfs_read+0x198/0x6c8
ksys_read+0xf4/0x1e0
__arm64_sys_read+0x70/0xa8
invoke_syscall+0x70/0x260
el0_svc_common.constprop.0+0xb0/0x280
do_el0_svc+0x44/0x60
el0_svc+0x34/0x68
el0t_64_sync_handler+0xb8/0xc0
el0t_64_sync+0x168/0x170

Allocated by task 188:
kasan_save_stack+0x28/0x50
kasan_set_track+0x28/0x38
kasan_save_alloc_info+0x20/0x30
__kasan_slab_alloc+0x6c/0x80
kmem_cache_alloc+0x15c/0x4a8
trace_create_new_event+0x84/0x348
__trace_add_new_event+0x18/0x88
event_trace_add_tracer+0xc4/0x1a0
trace_array_create_dir+0x6c/0x100
trace_array_create+0x2e8/0x568
instance_mkdir+0x48/0x80
tracefs_syscall_mkdir+0x90/0xe8
vfs_mkdir+0x3c4/0x610
do_mkdirat+0x144/0x200
__arm64_sys_mkdirat+0x8c/0xc0
invoke_syscall+0x70/0x260
el0_svc_common.constprop.0+0xb0/0x280
do_el0_svc+0x44/0x60
el0_svc+0x34/0x68
el0t_64_sync_handler+0xb8/0xc0
el0t_64_sync+0x168/0x170

Freed by task 191:
kasan_save_stack+0x28/0x50
kasan_set_track+0x28/0x38
kasan_save_free_info+0x34/0x58
__kasan_slab_free+0xe4/0x158
kmem_cache_free+0x19c/0x508
event_file_put+0xa0/0x120
remove_event_file_dir+0x180/0x320
event_trace_del_tracer+0xb0/0x180
__remove_instance+0x224/0x508
instance_rmdir+0x44/0x78
tracefs_syscall_rmdir+0xbc/0x140
vfs_rmdir+0x1cc/0x4c8
do_rmdir+0x220/0x2b8
__arm64_sys_unlinkat+0xc0/0x100
invoke_syscall+0x70/0x260
el0_svc_common.constprop.0+0xb0/0x280
do_el0_svc+0x44/0x60
el0_svc+0x34/0x68
el0t_64_sync_handler+0xb8/0xc0
el0t_64_sync+0x168/0x170

Suggested-by: Steven Rostedt <[email protected]>
Signed-off-by: Zheng Yejian <[email protected]>
---
kernel/trace/trace.c | 6 ++++++
kernel/trace/trace.h | 1 +
kernel/trace/trace_events_hist.c | 12 ++++++++----
3 files changed, 15 insertions(+), 4 deletions(-)

v3:
- As suggested by Steve, put the tracing_single_release_file_tr() into
trace.c as a non static function, put the prototype in trace.h.
Link: https://lore.kernel.org/all/[email protected]/

v2:
- Introduce tracing_single_release_file_tr() to add the missing call for
single_release() as suggested by Steve;
Link: https://lore.kernel.org/all/[email protected]/
- Slightly modify the commit message and comments.
- Link: https://lore.kernel.org/all/[email protected]/

v1:
Link: https://lore.kernel.org/all/[email protected]/

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index fbcd3bafb93e..fc1b2ada98a5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4964,6 +4964,12 @@ int tracing_release_file_tr(struct inode *inode, struct file *filp)
return 0;
}

+int tracing_single_release_file_tr(struct inode *inode, struct file *filp)
+{
+ tracing_release_file_tr(inode, filp);
+ return single_release(inode, filp);
+}
+
static int tracing_mark_open(struct inode *inode, struct file *filp)
{
stream_open(inode, filp);
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index b7f4ea25a194..0489e72c8169 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -617,6 +617,7 @@ int tracing_open_generic(struct inode *inode, struct file *filp);
int tracing_open_generic_tr(struct inode *inode, struct file *filp);
int tracing_open_file_tr(struct inode *inode, struct file *filp);
int tracing_release_file_tr(struct inode *inode, struct file *filp);
+int tracing_single_release_file_tr(struct inode *inode, struct file *filp);
bool tracing_is_disabled(void);
bool tracer_tracing_is_on(struct trace_array *tr);
void tracer_tracing_on(struct trace_array *tr);
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 1abc07fba1b9..5ecf3c8bde20 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -5623,10 +5623,12 @@ static int event_hist_open(struct inode *inode, struct file *file)
{
int ret;

- ret = security_locked_down(LOCKDOWN_TRACEFS);
+ ret = tracing_open_file_tr(inode, file);
if (ret)
return ret;

+ /* Clear private_data to avoid warning in single_open() */
+ file->private_data = NULL;
return single_open(file, hist_show, file);
}

@@ -5634,7 +5636,7 @@ const struct file_operations event_hist_fops = {
.open = event_hist_open,
.read = seq_read,
.llseek = seq_lseek,
- .release = single_release,
+ .release = tracing_single_release_file_tr,
};

#ifdef CONFIG_HIST_TRIGGERS_DEBUG
@@ -5900,10 +5902,12 @@ static int event_hist_debug_open(struct inode *inode, struct file *file)
{
int ret;

- ret = security_locked_down(LOCKDOWN_TRACEFS);
+ ret = tracing_open_file_tr(inode, file);
if (ret)
return ret;

+ /* Clear private_data to avoid warning in single_open() */
+ file->private_data = NULL;
return single_open(file, hist_debug_show, file);
}

@@ -5911,7 +5915,7 @@ const struct file_operations event_hist_debug_fops = {
.open = event_hist_debug_open,
.read = seq_read,
.llseek = seq_lseek,
- .release = single_release,
+ .release = tracing_single_release_file_tr,
};
#endif

--
2.25.1