Re: [PATCH] tracing: Have trace_event_file have ref counters

From: Google
Date: Wed Nov 01 2023 - 22:14:42 EST

Next message: Muchun Song: "Re: [PATCH v4 2/5] zswap: make shrinking memcg-aware"
Previous message: Google: "Re: [PATCH v6 7/8] eventfs: Remove special processing of dput() of events directory"
Next in thread: Steven Rostedt: "Re: [PATCH] tracing: Have trace_event_file have ref counters"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 31 Oct 2023 12:24:53 -0400
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:

> From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
>
> The following can crash the kernel:
>
> # cd /sys/kernel/tracing
> # echo 'p:sched schedule' > kprobe_events
> # exec 5>>events/kprobes/sched/enable
> # > kprobe_events
> # exec 5>&-
>
> The above commands:
>
> 1. Change directory to the tracefs directory
> 2. Create a kprobe event (doesn't matter what one)
> 3. Open bash file descriptor 5 on the enable file of the kprobe event
> 4. Delete the kprobe event (removes the files too)
> 5. Close the bash file descriptor 5
>
> The above causes a crash!
>
> BUG: kernel NULL pointer dereference, address: 0000000000000028
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 0 P4D 0
> Oops: 0000 [#1] PREEMPT SMP PTI
> CPU: 6 PID: 877 Comm: bash Not tainted 6.5.0-rc4-test-00008-g2c6b6b1029d4-dirty #186
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> RIP: 0010:tracing_release_file_tr+0xc/0x50
>
> What happens here is that the kprobe event creates a trace_event_file
> "file" descriptor that represents the file in tracefs to the event. It
> maintains state of the event (is it enabled for the given instance?).
> Opening the "enable" file gets a reference to the event "file" descriptor
> via the open file descriptor. When the kprobe event is deleted, the file is
> also deleted from the tracefs system which also frees the event "file"
> descriptor.

Ouch! I thought the file descriptor has been hold by the opened process.

>
> But as the tracefs file is still opened by user space, it will not be
> totally removed until the final dput() is called on it. But this is not
> true with the event "file" descriptor that is already freed. If the user
> does a write to or simply closes the file descriptor it will reference the
> event "file" descriptor that was just freed, causing a use-after-free bug.
>
> To solve this, add a ref count to the event "file" descriptor as well as a
> new flag called "FREED". The "file" will not be freed until the last
> reference is released. But the FREE flag will be set when the event is
> removed to prevent any more modifications to that event from happening,
> even if there's still a reference to the event "file" descriptor.
>
> Link: https://lore.kernel.org/linux-trace-kernel/20231031000031.1e705592@xxxxxxxxxxxxxxxxxx/
>

Looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>

BTW, can we add some tracefs selftests to ftracetest or independent test?

Thanks,

> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: f5ca233e2e66d ("tracing: Increase trace array ref count on enable and filter files")
> Reported-by: Beau Belgrave <beaub@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
> ---
> include/linux/trace_events.h | 4 ++++
> kernel/trace/trace.c | 15 +++++++++++++++
> kernel/trace/trace.h | 3 +++
> kernel/trace/trace_events.c | 31 ++++++++++++++++++++++++++----
> kernel/trace/trace_events_filter.c | 3 +++
> 5 files changed, 52 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> index 12207dc6722d..696f8dc4aa53 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -492,6 +492,7 @@ enum {
> EVENT_FILE_FL_TRIGGER_COND_BIT,
> EVENT_FILE_FL_PID_FILTER_BIT,
> EVENT_FILE_FL_WAS_ENABLED_BIT,
> + EVENT_FILE_FL_FREED_BIT,
> };
>
> extern struct trace_event_file *trace_get_event_file(const char *instance,
> @@ -630,6 +631,7 @@ extern int __kprobe_event_add_fields(struct dynevent_cmd *cmd, ...);
> * TRIGGER_COND - When set, one or more triggers has an associated filter
> * PID_FILTER - When set, the event is filtered based on pid
> * WAS_ENABLED - Set when enabled to know to clear trace on module removal
> + * FREED - File descriptor is freed, all fields should be considered invalid
> */
> enum {
> EVENT_FILE_FL_ENABLED = (1 << EVENT_FILE_FL_ENABLED_BIT),
> @@ -643,6 +645,7 @@ enum {
> EVENT_FILE_FL_TRIGGER_COND = (1 << EVENT_FILE_FL_TRIGGER_COND_BIT),
> EVENT_FILE_FL_PID_FILTER = (1 << EVENT_FILE_FL_PID_FILTER_BIT),
> EVENT_FILE_FL_WAS_ENABLED = (1 << EVENT_FILE_FL_WAS_ENABLED_BIT),
> + EVENT_FILE_FL_FREED = (1 << EVENT_FILE_FL_FREED_BIT),
> };
>
> struct trace_event_file {
> @@ -671,6 +674,7 @@ struct trace_event_file {
> * caching and such. Which is mostly OK ;-)
> */
> unsigned long flags;
> + atomic_t ref; /* ref count for opened files */
> atomic_t sm_ref; /* soft-mode reference counter */
> atomic_t tm_ref; /* trigger-mode reference counter */
> };
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 2539cfc20a97..9aebf904ff97 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4978,6 +4978,20 @@ int tracing_open_file_tr(struct inode *inode, struct file *filp)
> if (ret)
> return ret;
>
> + mutex_lock(&event_mutex);
> +
> + /* Fail if the file is marked for removal */
> + if (file->flags & EVENT_FILE_FL_FREED) {
> + trace_array_put(file->tr);
> + ret = -ENODEV;
> + } else {
> + event_file_get(file);
> + }
> +
> + mutex_unlock(&event_mutex);
> + if (ret)
> + return ret;
> +
> filp->private_data = inode->i_private;
>
> return 0;
> @@ -4988,6 +5002,7 @@ int tracing_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 0;
> }
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 0e1405abf4f7..b7f4ea25a194 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1669,6 +1669,9 @@ extern void event_trigger_unregister(struct event_command *cmd_ops,
> char *glob,
> struct event_trigger_data *trigger_data);
>
> +extern void event_file_get(struct trace_event_file *file);
> +extern void event_file_put(struct trace_event_file *file);
> +
> /**
> * struct event_trigger_ops - callbacks for trace event triggers
> *
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index f9e3e24d8796..f29e815ca5b2 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -990,13 +990,35 @@ static void remove_subsystem(struct trace_subsystem_dir *dir)
> }
> }
>
> +void event_file_get(struct trace_event_file *file)
> +{
> + atomic_inc(&file->ref);
> +}
> +
> +void event_file_put(struct trace_event_file *file)
> +{
> + if (WARN_ON_ONCE(!atomic_read(&file->ref))) {
> + if (file->flags & EVENT_FILE_FL_FREED)
> + kmem_cache_free(file_cachep, file);
> + return;
> + }
> +
> + if (atomic_dec_and_test(&file->ref)) {
> + /* Count should only go to zero when it is freed */
> + if (WARN_ON_ONCE(!(file->flags & EVENT_FILE_FL_FREED)))
> + return;
> + kmem_cache_free(file_cachep, file);
> + }
> +}
> +
> static void remove_event_file_dir(struct trace_event_file *file)
> {
> eventfs_remove_dir(file->ei);
> list_del(&file->list);
> remove_subsystem(file->system);
> free_event_filter(file->filter);
> - kmem_cache_free(file_cachep, file);
> + file->flags |= EVENT_FILE_FL_FREED;
> + event_file_put(file);
> }
>
> /*
> @@ -1369,7 +1391,7 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
> flags = file->flags;
> mutex_unlock(&event_mutex);
>
> - if (!file)
> + if (!file || flags & EVENT_FILE_FL_FREED)
> return -ENODEV;
>
> if (flags & EVENT_FILE_FL_ENABLED &&
> @@ -1403,7 +1425,7 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
> ret = -ENODEV;
> mutex_lock(&event_mutex);
> file = event_file_data(filp);
> - if (likely(file)) {
> + if (likely(file && !(file->flags & EVENT_FILE_FL_FREED))) {
> ret = tracing_update_buffers(file->tr);
> if (ret < 0) {
> mutex_unlock(&event_mutex);
> @@ -1683,7 +1705,7 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
>
> mutex_lock(&event_mutex);
> file = event_file_data(filp);
> - if (file)
> + if (file && !(file->flags & EVENT_FILE_FL_FREED))
> print_event_filter(file, s);
> mutex_unlock(&event_mutex);
>
> @@ -2902,6 +2924,7 @@ trace_create_new_event(struct trace_event_call *call,
> atomic_set(&file->tm_ref, 0);
> INIT_LIST_HEAD(&file->triggers);
> list_add(&file->list, &tr->events);
> + event_file_get(file);
>
> return file;
> }
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 33264e510d16..0c611b281a5b 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -2349,6 +2349,9 @@ int apply_event_filter(struct trace_event_file *file, char *filter_string)
> struct event_filter *filter = NULL;
> int err;
>
> + if (file->flags & EVENT_FILE_FL_FREED)
> + return -ENODEV;
> +
> if (!strcmp(strstrip(filter_string), "0")) {
> filter_disable(file);
> filter = event_filter(file);
> --
> 2.42.0
>

--
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>

Next message: Muchun Song: "Re: [PATCH v4 2/5] zswap: make shrinking memcg-aware"
Previous message: Google: "Re: [PATCH v6 7/8] eventfs: Remove special processing of dput() of events directory"
Next in thread: Steven Rostedt: "Re: [PATCH] tracing: Have trace_event_file have ref counters"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]