[PATCH] drivers/perf: riscv: Fix racy hw.idx use in user access paths

[PATCH] drivers/perf: riscv: Fix racy hw.idx use in user access paths

[Li Jinyue]

riscv_pmu_event_idx() and arch_perf_update_userpage() are called from
perf_mmap() through perf_event_update_userpage(), and
pmu_sbi_set_scounteren()/pmu_sbi_reset_scounteren() run on every CPU in
mm_cpumask() through IPIs sent by pmu_sbi_event_mapped() and
pmu_sbi_event_unmapped(). None of them hold the event context lock, so
the event can be scheduled in or out on another CPU at the same time,
and riscv_pmu_add()/riscv_pmu_del() change hw.idx underneath them.

Taking the context lock is not an option: perf_event_update_userpage()
is also called from riscv_pmu_add() and riscv_pmu_del(), which already
hold it.

Commit 3fec323339a4 ("drivers: perf: Fix panic in riscv SBI mmap
support") added hw.idx != -1 checks to the scounteren helpers and to
riscv_perf_user_access(), but hw.idx is loaded again afterwards to
index pmu_ctr_list, so the check does not cover the access.
riscv_pmu_event_idx() has no check and relies on perf_event_index(),
which only looks at the software state (event->state and hw.state) and
not at hw.idx. event_sched_in() marks the event ACTIVE before calling
pmu::add(), and hw.state is still zero on the first schedule-in, so
perf_event_index() can call into the driver with hw.idx == -1.
event_sched_out() likewise calls pmu::del() before the state changes.

syzkaller hits this on QEMU with KASAN. In every report the faulting
address is pmu_ctr_list[-1]:

  BUG: KASAN: slab-out-of-bounds in pmu_sbi_csr_index+0x80/0x82
  Read of size 8 at addr ff60000009933bf8 by task syz.0.5217/27313
  Call Trace:
   pmu_sbi_csr_index+0x80/0x82
   riscv_pmu_event_idx+0x9c/0xd8
   perf_event_update_userpage+0x544/0x6ca
   riscv_pmu_event_mapped+0x6e/0x98
   perf_mmap+0xbcc/0x22ae
   __mmap_region+0x1128/0x2398
   mmap_region+0x3cc/0x466
   do_mmap+0xe7c/0x162c
   vm_mmap_pgoff+0x1e8/0x362
   ksys_mmap_pgoff+0x338/0x402
   __riscv_sys_mmap+0x11c/0x18c

The same read is also reported in pmu_sbi_ctr_get_width() from
arch_perf_update_userpage(), in pmu_sbi_set_scounteren() from the IPI
handler and in pmu_sbi_reset_scounteren() from perf_mmap_close().

Load hw.idx once with READ_ONCE() in these paths, check that value and
use it for the lookup. Change the csr_index() callback to take the
counter index so that the value checked is the value used; neither
backend needs anything else from the event. Use WRITE_ONCE() for the
stores in riscv_pmu_add() and riscv_pmu_del().

Fixes: cc4c07c89aad ("drivers: perf: Implement perf event mmap support in the SBI backend")
Cc: stable@vger.kernel.org
Signed-off-by: Li Jinyue <jinyue.li@linux.alibaba.com>
---
 drivers/perf/riscv_pmu.c        | 22 ++++++++++++++--------
 drivers/perf/riscv_pmu_legacy.c |  4 ++--
 drivers/perf/riscv_pmu_sbi.c    | 14 ++++++++------
 include/linux/perf/riscv_pmu.h  |  2 +-
 4 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/perf/riscv_pmu.c b/drivers/perf/riscv_pmu.c
index 8e3cd0f35336..0ec4114595b0 100644
--- a/drivers/perf/riscv_pmu.c
+++ b/drivers/perf/riscv_pmu.c
@@ -18,18 +18,23 @@
 
 #include <asm/sbi.h>
 
-static bool riscv_perf_user_access(struct perf_event *event)
+static bool riscv_perf_user_access(struct perf_event *event, int idx)
 {
 	return ((event->attr.type == PERF_TYPE_HARDWARE) ||
 		(event->attr.type == PERF_TYPE_HW_CACHE) ||
 		(event->attr.type == PERF_TYPE_RAW)) &&
 		!!(event->hw.flags & PERF_EVENT_FLAG_USER_READ_CNT) &&
-		(event->hw.idx != -1);
+		(idx != -1);
 }
 
 void arch_perf_update_userpage(struct perf_event *event,
 			       struct perf_event_mmap_page *userpg, u64 now)
 {
+	/*
+	 * hw.idx is updated by pmu::add()/del() on another CPU, so take a
+	 * snapshot and use it for both the check and the lookup.
+	 */
+	int idx = READ_ONCE(event->hw.idx);
 	struct clock_read_data *rd;
 	unsigned int seq;
 	u64 ns;
@@ -37,7 +42,7 @@ void arch_perf_update_userpage(struct perf_event *event,
 	userpg->cap_user_time = 0;
 	userpg->cap_user_time_zero = 0;
 	userpg->cap_user_time_short = 0;
-	userpg->cap_user_rdpmc = riscv_perf_user_access(event);
+	userpg->cap_user_rdpmc = riscv_perf_user_access(event, idx);
 
 	/*
 	 * The counters are 64-bit but the priv spec doesn't mandate all the
@@ -45,7 +50,7 @@ void arch_perf_update_userpage(struct perf_event *event,
 	 * the cpu vendor.
 	 */
 	if (userpg->cap_user_rdpmc)
-		userpg->pmc_width = to_riscv_pmu(event->pmu)->ctr_get_width(event->hw.idx) + 1;
+		userpg->pmc_width = to_riscv_pmu(event->pmu)->ctr_get_width(idx) + 1;
 
 	do {
 		rd = sched_clock_read_begin(&seq);
@@ -265,7 +270,7 @@ static int riscv_pmu_add(struct perf_event *event, int flags)
 	if (idx < 0)
 		return idx;
 
-	hwc->idx = idx;
+	WRITE_ONCE(hwc->idx, idx);
 	cpuc->events[idx] = event;
 	cpuc->n_events++;
 	hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
@@ -293,7 +298,7 @@ static void riscv_pmu_del(struct perf_event *event, int flags)
 	if (rvpmu->ctr_clear_idx)
 		rvpmu->ctr_clear_idx(event);
 	perf_event_update_userpage(event);
-	hwc->idx = -1;
+	WRITE_ONCE(hwc->idx, -1);
 }
 
 static void riscv_pmu_read(struct perf_event *event)
@@ -353,12 +358,13 @@ static int riscv_pmu_event_init(struct perf_event *event)
 static int riscv_pmu_event_idx(struct perf_event *event)
 {
 	struct riscv_pmu *rvpmu = to_riscv_pmu(event->pmu);
+	int idx = READ_ONCE(event->hw.idx);
 
-	if (!(event->hw.flags & PERF_EVENT_FLAG_USER_READ_CNT))
+	if (!(event->hw.flags & PERF_EVENT_FLAG_USER_READ_CNT) || idx == -1)
 		return 0;
 
 	if (rvpmu->csr_index)
-		return rvpmu->csr_index(event) + 1;
+		return rvpmu->csr_index(idx) + 1;
 
 	return 0;
 }
diff --git a/drivers/perf/riscv_pmu_legacy.c b/drivers/perf/riscv_pmu_legacy.c
index 1b8e4789cb3c..e540bb798fb6 100644
--- a/drivers/perf/riscv_pmu_legacy.c
+++ b/drivers/perf/riscv_pmu_legacy.c
@@ -76,9 +76,9 @@ static void pmu_legacy_ctr_start(struct perf_event *event, u64 ival)
 	local64_set(&hwc->prev_count, initial_val);
 }
 
-static uint8_t pmu_legacy_csr_index(struct perf_event *event)
+static uint8_t pmu_legacy_csr_index(int idx)
 {
-	return event->hw.idx;
+	return idx;
 }
 
 static void pmu_legacy_event_mapped(struct perf_event *event, struct mm_struct *mm)
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 2991dd92def2..2360ee5d8843 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -530,9 +530,9 @@ int riscv_pmu_get_hpm_info(u32 *hw_ctr_width, u32 *num_hw_ctr)
 }
 EXPORT_SYMBOL_GPL(riscv_pmu_get_hpm_info);
 
-static uint8_t pmu_sbi_csr_index(struct perf_event *event)
+static uint8_t pmu_sbi_csr_index(int idx)
 {
-	return pmu_ctr_list[event->hw.idx].csr - CSR_CYCLE;
+	return pmu_ctr_list[idx].csr - CSR_CYCLE;
 }
 
 static unsigned long pmu_sbi_get_filter_flags(struct perf_event *event)
@@ -809,19 +809,21 @@ static u64 pmu_sbi_ctr_read(struct perf_event *event)
 static void pmu_sbi_set_scounteren(void *arg)
 {
 	struct perf_event *event = (struct perf_event *)arg;
+	int idx = READ_ONCE(event->hw.idx);
 
-	if (event->hw.idx != -1)
+	if (idx != -1)
 		csr_write(CSR_SCOUNTEREN,
-			  csr_read(CSR_SCOUNTEREN) | BIT(pmu_sbi_csr_index(event)));
+			  csr_read(CSR_SCOUNTEREN) | BIT(pmu_sbi_csr_index(idx)));
 }
 
 static void pmu_sbi_reset_scounteren(void *arg)
 {
 	struct perf_event *event = (struct perf_event *)arg;
+	int idx = READ_ONCE(event->hw.idx);
 
-	if (event->hw.idx != -1)
+	if (idx != -1)
 		csr_write(CSR_SCOUNTEREN,
-			  csr_read(CSR_SCOUNTEREN) & ~BIT(pmu_sbi_csr_index(event)));
+			  csr_read(CSR_SCOUNTEREN) & ~BIT(pmu_sbi_csr_index(idx)));
 }
 
 static void pmu_sbi_ctr_start(struct perf_event *event, u64 ival)
diff --git a/include/linux/perf/riscv_pmu.h b/include/linux/perf/riscv_pmu.h
index ecaa40370830..4690a5646ae9 100644
--- a/include/linux/perf/riscv_pmu.h
+++ b/include/linux/perf/riscv_pmu.h
@@ -66,7 +66,7 @@ struct riscv_pmu {
 	void		(*event_init)(struct perf_event *event);
 	void		(*event_mapped)(struct perf_event *event, struct mm_struct *mm);
 	void		(*event_unmapped)(struct perf_event *event, struct mm_struct *mm);
-	uint8_t		(*csr_index)(struct perf_event *event);
+	uint8_t		(*csr_index)(int idx);
 
 	struct cpu_hw_events	__percpu *hw_events;
 	struct hlist_node	node;
-- 
2.25.1