[PATCH v2] perf/bpf: Fix data races in BPF perf event handling

Deepanshu Kartikey <[email protected]>
Newsgroups org.kernel.vger.linux-trace-kernel,org.kernel.vger.bpf,org.kernel.vger.linux-kernel,org.kernel.vger.linux-perf-users
Message-ID <[email protected]>
Fix multiple data races involving event->prog field:

1. __perf_event_overflow() reads event->prog twice without
   synchronization creating a TOCTOU race. Fix by using READ_ONCE()
   to capture prog into a local variable and pass it directly to
   bpf_overflow_handler() to avoid a second read inside that function.

2. perf_event_set_bpf_handler() and perf_event_free_bpf_handler()
   perform plain writes to event->prog without WRITE_ONCE(), failing
   to pair with the READ_ONCE() in __perf_event_overflow(). Fix by
   using WRITE_ONCE() in all write paths including
   perf_event_detach_bpf_prog().

3. perf_event_alloc() reads parent_event->prog locklessly during
   fork() which can race with a concurrent detach clearing and freeing
   the prog, potentially causing a NULL pointer dereference or
   use-after-free in bpf_prog_inc(). Fix by holding bpf_event_mutex
   when inheriting the BPF program. Make bpf_event_mutex non-static
   and declare it extern in perf_event.h so it is accessible from
   kernel/events/core.c.

Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=651d2774bd1d8395595f
Link: https://lore.kernel.org/all/[email protected]/T/ [v1]
Signed-off-by: Deepanshu Kartikey <[email protected]>
---
v2:
- Pass validated prog pointer directly to bpf_overflow_handler()
  to avoid TOCTOU race from second READ_ONCE() inside the handler
- Add WRITE_ONCE() to perf_event_set_bpf_handler() and
  perf_event_free_bpf_handler() to pair with READ_ONCE() in overflow
- Fix lockless access in perf_event_alloc() during fork() by holding
  bpf_event_mutex to prevent concurrent detach UAF
- Make bpf_event_mutex non-static and export via perf_event.h
---
 include/linux/perf_event.h |  2 ++
 kernel/events/core.c       | 23 +++++++++++++----------
 kernel/trace/bpf_trace.c   |  4 ++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 48d851fbd8ea..5c6dabb6dccb 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -2136,4 +2136,6 @@ static inline void perf_lopwr_cb(bool mode)
 }
 #endif
 
+extern struct mutex bpf_event_mutex;
+
 #endif /* _LINUX_PERF_EVENT_H */
diff --git a/kernel/events/core.c b/kernel/events/core.c
index ba5bd6a78fe7..6f8d3b57fa01 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10652,20 +10652,19 @@ static inline bool sample_is_allowed(struct perf_event *event, struct pt_regs *r
 #ifdef CONFIG_BPF_SYSCALL
 static int bpf_overflow_handler(struct perf_event *event,
 				struct perf_sample_data *data,
-				struct pt_regs *regs)
+				struct pt_regs *regs,
+				struct bpf_prog *prog)
 {
 	struct bpf_perf_event_data_kern ctx = {
 		.data = data,
 		.event = event,
 	};
-	struct bpf_prog *prog;
 	int ret = 0;
 
 	ctx.regs = perf_arch_bpf_user_pt_regs(regs);
 	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1))
 		goto out;
 	rcu_read_lock();
-	prog = READ_ONCE(event->prog);
 	if (prog) {
 		perf_prepare_sample(data, event, regs);
 		ret = bpf_prog_run(prog, &ctx);
@@ -10708,7 +10707,7 @@ static inline int perf_event_set_bpf_handler(struct perf_event *event,
 		return -EPROTO;
 	}
 
-	event->prog = prog;
+	WRITE_ONCE(event->prog, prog);
 	event->bpf_cookie = bpf_cookie;
 	return 0;
 }
@@ -10720,7 +10719,7 @@ static inline void perf_event_free_bpf_handler(struct perf_event *event)
 	if (!prog)
 		return;
 
-	event->prog = NULL;
+	WRITE_ONCE(event->prog, NULL);
 	bpf_prog_put(prog);
 }
 #else
@@ -10753,6 +10752,7 @@ static int __perf_event_overflow(struct perf_event *event,
 {
 	int events = atomic_read(&event->event_limit);
 	int ret = 0;
+	struct bpf_prog *prog;
 
 	/*
 	 * Non-sampling counters might still use the PMI to fold short
@@ -10766,8 +10766,9 @@ static int __perf_event_overflow(struct perf_event *event,
 	if (event->attr.aux_pause)
 		perf_event_aux_pause(event->aux_event, true);
 
-	if (event->prog && event->prog->type == BPF_PROG_TYPE_PERF_EVENT &&
-	    !bpf_overflow_handler(event, data, regs))
+	prog = READ_ONCE(event->prog);
+	if (prog && prog->type == BPF_PROG_TYPE_PERF_EVENT &&
+	    !bpf_overflow_handler(event, data, regs, prog))
 		goto out;
 
 	/*
@@ -13433,12 +13434,15 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 		overflow_handler = parent_event->overflow_handler;
 		context = parent_event->overflow_handler_context;
 #if defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_EVENT_TRACING)
-		if (parent_event->prog) {
-			struct bpf_prog *prog = parent_event->prog;
-
+		struct bpf_prog *prog;
+
+		mutex_lock(&bpf_event_mutex);
+		prog = parent_event->prog;
+		if (prog) {
 			bpf_prog_inc(prog);
 			event->prog = prog;
 		}
+		mutex_unlock(&bpf_event_mutex);
 #endif
 	}
 
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 75495a5c3507..026b5c492115 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1944,7 +1944,7 @@ const struct bpf_verifier_ops perf_event_verifier_ops = {
 const struct bpf_prog_ops perf_event_prog_ops = {
 };
 
-static DEFINE_MUTEX(bpf_event_mutex);
+DEFINE_MUTEX(bpf_event_mutex);
 
 #define BPF_TRACE_MAX_PROGS 64
 
@@ -2018,7 +2018,7 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
 
 put:
 	prog = event->prog;
-	event->prog = NULL;
+	WRITE_ONCE(event->prog, NULL);
 
 unlock:
 	mutex_unlock(&bpf_event_mutex);
-- 
2.43.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.