[PATCH] accel: ethosu: Fix use-after-free of the active perfmon

[PATCH] accel: ethosu: Fix use-after-free of the active perfmon

[Jiwoo Kim]

ethosu_perfmon_start() stores the perfmon in perfmon_state.active
without taking a reference, and nothing stops the counters when the job
that started them completes: the perfmon stays active until a later job
runs with a different perfmon or none, GET_VALUES or CLEAR_GLOBAL stops
it, or it is destroyed. Meanwhile it is only kept alive by the
references held by the file's perfmon xarray, the global perfmon and
the jobs using it.

If userspace destroys a perfmon while a job using it is still queued,
ethosu_perfmon_delete() can only stop it if it happens to be active at
that point, and the queued job makes it active when it runs. When the
last such job is released, the perfmon is freed, but
perfmon_state.active still points to it. The next job's
ethosu_switch_perfmon() then passes it to ethosu_perfmon_stop_locked(),
which reads ncounters and updates values[] of the freed perfmon.

Make perfmon_state.active hold its own reference: take it in
ethosu_perfmon_start() and drop it in ethosu_perfmon_stop_locked() when
the perfmon stops being active. As a perfmon can now be kept alive by
perfmon_state.active alone, also drop that reference when the DRM
device is released. The counters are still left running after a job
completes, as before.

The problem was found through AI-assisted source code review. The fix
is compile-tested only and there is no reproducer, as I don't have
Ethos-U hardware.

Fixes: 17cdb54644e7 ("accel: ethosu: Add performance counter support")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Jiwoo Kim <arpe1618@gmail.com>
---

Notes:
    This keeps the counters running after the job that started them
    completes, as the driver does today, and only fixes the lifetime of the
    active perfmon. v3d instead stops the job's perfmon when the job
    completes; I can do that instead if you prefer, but it would also stop
    the idle and cycle counters at that point.
    
    It applies to drm-misc-fixes, and on top of v4 of "accel: ethosu:
    Another batch of fixes".

 drivers/accel/ethosu/ethosu_drv.c     | 20 ++++++++++++++++++++
 drivers/accel/ethosu/ethosu_perfmon.c | 15 +++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index 4a24bbd2bb..b6778196c9 100644
--- a/drivers/accel/ethosu/ethosu_drv.c
+++ b/drivers/accel/ethosu/ethosu_drv.c
@@ -341,6 +341,21 @@ static int ethosu_init(struct ethosu_device *ethosudev)
 	return 0;
 }
 
+static void ethosu_put_active_perfmon(struct drm_device *ddev, void *res)
+{
+	struct ethosu_device *ethosudev = to_ethosu_device(ddev);
+	struct ethosu_perfmon *perfmon = ethosudev->perfmon_state.active;
+
+	/*
+	 * A perfmon left active holds a reference, see ethosu_perfmon_start().
+	 * This runs at the final drm_dev_put(), possibly long after unbind,
+	 * when no job or file can use the perfmon any more: just drop the
+	 * reference, without touching the hardware.
+	 */
+	ethosudev->perfmon_state.active = NULL;
+	ethosu_perfmon_put(perfmon);
+}
+
 static int ethosu_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -367,6 +382,11 @@ static int ethosu_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = drmm_add_action_or_reset(&ethosudev->base,
+				       ethosu_put_active_perfmon, NULL);
+	if (ret)
+		return ret;
+
 	ret = ethosu_job_init(ethosudev);
 	if (ret)
 		return ret;
diff --git a/drivers/accel/ethosu/ethosu_perfmon.c b/drivers/accel/ethosu/ethosu_perfmon.c
index 26f625374f..69c0967f87 100644
--- a/drivers/accel/ethosu/ethosu_perfmon.c
+++ b/drivers/accel/ethosu/ethosu_perfmon.c
@@ -54,9 +54,20 @@ void ethosu_perfmon_start(struct ethosu_device *ethosu, struct ethosu_perfmon *p
 	writel_relaxed(mask, ethosu->pmu_regs + NPU_REG_PMCNTENSET);
 	writel_relaxed(PMCR_CNT_EN | PMCR_EVENT_CNT_RST | PMCR_CYCLE_CNT_RST,
 		ethosu->pmu_regs + NPU_REG_PMCR);
+
+	/*
+	 * The counters keep running after the job that started them
+	 * completes, so the perfmon can stay active after that job and its
+	 * handle are gone. Hold a reference for as long as it is active.
+	 */
+	ethosu_perfmon_get(perfmon);
 	ethosu->perfmon_state.active = perfmon;
 }
 
+/*
+ * Stops @perfmon if it is the active perfmon. This drops the reference
+ * held by perfmon_state.active, which can free @perfmon.
+ */
 void ethosu_perfmon_stop_locked(struct ethosu_device *ethosu, struct ethosu_perfmon *perfmon,
 				bool capture)
 {
@@ -73,6 +84,7 @@ void ethosu_perfmon_stop_locked(struct ethosu_device *ethosu, struct ethosu_perf
 
 	if (!pm_runtime_get_if_active(ethosu->base.dev)) {
 		ethosu->perfmon_state.active = NULL;
+		ethosu_perfmon_put(perfmon);
 		return;
 	}
 
@@ -94,6 +106,9 @@ void ethosu_perfmon_stop_locked(struct ethosu_device *ethosu, struct ethosu_perf
 	ethosu->perfmon_state.active = NULL;
 
 	pm_runtime_put(ethosu->base.dev);
+
+	/* Drop the reference taken by ethosu_perfmon_start(). */
+	ethosu_perfmon_put(perfmon);
 }
 
 void ethosu_perfmon_stop(struct ethosu_device *ethosu, struct ethosu_perfmon *perfmon,

base-commit: bafda823590ddf7031ecb765d673572fae7ab44a
-- 
2.43.0