tracing: More fixes for v7.2
Subsystems (2)
cover tracing: More fixes for v7.2
tracing fixes for 7.2:
- Fix use-after-free in user_event_mm_dup()
When the enabler is removed from the link list, it is freed immediately. But it is protected via RCU and needs to be freed after an RCU grace period. Use queue_rcu_work() so that the event_mutex can also be taken as user_event_put() takes the mutex on the last reference is released.
- Free type string in error path of parse_synth_field()
There's an error path in parse_synth_field() where the allocated type string is not freed.
git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
trace/fixes
Head SHA1: 05074bb90af94f2acbbc8f8ffaa507f914c273c8
Michael Bommarito (1):
tracing/user_events: Fix use-after-free in user_event_mm_dup()
Yu Peng (1):
tracing/synthetic: Free type string on error path
----
kernel/trace/trace_events_synth.c | 4 +++- kernel/trace/trace_events_user.c | 39 ++++++++++++++++++++++++++++++++------- 2 files changed, 35 insertions(+), 8 deletions(-)
PATCH 1/2 tracing/user_events: Fix use-after-free in user_event_mm_dup()
From: Michael Bommarito <[email protected]>
user_event_mm_dup() walks the parent mm's enabler list locklessly under rcu_read_lock() during fork() (from copy_process()); it does not take event_mutex:
rcu_read_lock(); list_for_each_entry_rcu(enabler, &old_mm->enablers, mm_enablers_link) enabler->event = user_event_get(orig->event);
user_event_enabler_destroy() removes an enabler from that list with list_del_rcu() and then, without waiting for a grace period, drops the enabler's user_event reference with user_event_put() and frees the enabler with kfree(). A reader that loaded the enabler before the list_del_rcu() can still be walking it, which leads to two use-after-frees:
- kfree(enabler) frees the enabler while that reader dereferences enabler->event.
- user_event_put() may drop the last reference to the user_event, which is then freed (via delayed_destroy_user_event() on a work queue), while the same reader does user_event_get(orig->event) on it.
Both are reachable by an unprivileged task that can open user_events_data:
one multithreaded process that registers an enabler and then concurrently unregisters it and calls fork() triggers the race. KASAN reports a slab-use-after-free in user_event_mm_dup() during clone(), with a "refcount_t: addition on 0" warning when the user_event is freed.
The enabler use-after-free was found first; the user_event one was reported by XIAO WU, and the earlier enabler-only fix did not address it.
Defer both the user_event_put() and the kfree(enabler) to a work item queued with queue_rcu_work(), so they run only after an RCU grace period, once all readers walking the enabler list have finished. The put must run in process context because user_event_put() takes event_mutex on the last reference, so a work queue is used rather than call_rcu(). The now-unlocked put lets the locked argument of user_event_enabler_destroy() be removed; all callers are updated.
--- kernel/trace/trace_events_user.c | 39 ++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 7 deletions(-)
kernel/trace/trace_events_user.c +32 -7
@@ -109,6 +109,9 @@ struct user_event_enabler {/* Track enable bit, flags, etc. Aligned for bitops. */ unsigned long values; + +/* Defer the event put and enabler free past an RCU grace period. */ +struct rcu_work put_rwork; };/* Bits 0-5 are for the bit to update upon enable/disable (0-63 allowed) */
@@ -396,17 +399,39 @@ static struct user_event_group *user_event_group_create(void)return NULL ; }; -static void user_event_enabler_destroy (struct user_event_enabler *enabler, -bool locked) +static void delayed_user_event_enabler_put (struct work_struct *work) { -list_del_rcu (&enabler->mm_enablers_link); +struct user_event_enabler *enabler =container_of (to_rcu_work (work), + struct user_event_enabler, put_rwork);/* No longer tracking the event via the enabler */ -user_event_put (enabler->event, locked); +user_event_put (enabler->event, false); +/* Run from queue_rcu_work(), the RCU grace period has elapsed */ kfree (enabler); } +static void user_event_enabler_destroy (struct user_event_enabler *enabler) +{ +list_del_rcu (&enabler->mm_enablers_link); + +/* +* The enabler is removed from an RCU-traversed list +* (user_event_mm_dup() walks mm->enablers under rcu_read_lock() only), +* and readers there dereference enabler->event and take a new ref on +* it. Both the put of that event reference and the free of the enabler +* therefore have to wait for a grace period so no reader can be looking +* at the enabler or racing the last put of its event. +* +* The put itself must not run in RCU context: when it drops the last +* reference user_event_put() takes event_mutex, which cannot be taken +* from a softirq/RCU callback. Defer both to a work item scheduled +* after a grace period via queue_rcu_work(). +*/ +INIT_RCU_WORK (&enabler->put_rwork, delayed_user_event_enabler_put); +queue_rcu_work (system_percpu_wq, &enabler->put_rwork); +} +static int user_event_mm_fault_in (struct user_event_mm *mm,unsigned long uaddr,int attempt) {
@@ -464,7 +489,7 @@ static void user_event_enabler_fault_fixup(struct work_struct *work)/* User asked for enabler to be removed during fault */ if (test_bit (ENABLE_VAL_FREEING_BIT ,ENABLE_BITOPS (enabler))) { -user_event_enabler_destroy (enabler, true); +user_event_enabler_destroy (enabler); goto out; }
@@ -764,7 +789,7 @@ static void user_event_mm_destroy(struct user_event_mm *mm)struct user_event_enabler *enabler, *next;list_for_each_entry_safe (enabler, next, &mm->enablers, mm_enablers_link) -user_event_enabler_destroy (enabler, false); +user_event_enabler_destroy (enabler);mmdrop (mm->mm);kfree (mm);
@@ -2645,7 +2670,7 @@ static long user_events_ioctl_unreg(unsigned long uarg) flags |= enabler->values &ENABLE_VAL_COMPAT_MASK ;if (!test_bit (ENABLE_VAL_FAULTING_BIT ,ENABLE_BITOPS (enabler))) -user_event_enabler_destroy (enabler, true); +user_event_enabler_destroy (enabler);/* Removed at least one */ ret =0 ;
Signature · 1 line
2.53.0
PATCH 2/2 tracing/synthetic: Free type string on error path
In reply to Steven RostedtFrom: Yu Peng <[email protected]>
parse_synth_field() builds a "__data_loc ..." type string before assigning it to field->type. If the seq_buf check fails, the common cleanup cannot free the temporary string. Free it before leaving.
--- kernel/trace/trace_events_synth.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
kernel/trace/trace_events_synth.c +3 -1
@@ -839,8 +839,10 @@ static struct synth_field *parse_synth_field(int argc, char **argv,seq_buf_puts (&s,"__data_loc " );seq_buf_puts (&s, field->type); -if (WARN_ON_ONCE (!seq_buf_buffer_left (&s))) +if (WARN_ON_ONCE (!seq_buf_buffer_left (&s))) { +kfree (type); goto free; + } s.buffer[s.len] ='\0' ;kfree (field->type);
Signature · 1 line
2.53.0