On Tue, 31 Oct 2023 12:24:53 -0400 Steven Rostedt <rost...@goodmis.org> wrote:
> From: "Steven Rostedt (Google)" <rost...@goodmis.org> > > 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.1e705...@gandalf.local.home/ > Looks good to me. Reviewed-by: Masami Hiramatsu (Google) <mhira...@kernel.org> BTW, can we add some tracefs selftests to ftracetest or independent test? Thanks, > Cc: sta...@vger.kernel.org > Fixes: f5ca233e2e66d ("tracing: Increase trace array ref count on enable and > filter files") > Reported-by: Beau Belgrave <be...@linux.microsoft.com> > Signed-off-by: Steven Rostedt (Google) <rost...@goodmis.org> > --- > 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) <mhira...@kernel.org>
