[PATCH v2] drm/i915/gt: Fix request use-after-free in heartbeat()

[PATCH v2] drm/i915/gt: Fix request use-after-free in heartbeat()

[Andi Shyti]

From: Shuangpeng Bai <shuangpeng.kernel@gmail.com>

heartbeat() detaches engine->heartbeat.systole with xchg() before
checking whether the request has completed. If the request is complete,
dropping the detached systole reference may free it. Concurrently,
idle_pulse() can observe the empty slot and publish a newer request.

The worker then sees a non-NULL engine->heartbeat.systole, but continues
to dereference its stale local rq. The newer request can therefore make
the shared-slot check succeed after the old request has been freed,
causing use-after-free accesses to emitted_jiffies, submit, and sched.

If the detached request is incomplete, heartbeat() can also overwrite a
newer request published by idle_pulse() when it unconditionally restores
the old pointer.

Use cmpxchg() both when idle_pulse() publishes a request and when
heartbeat() restores the detached request. Drop the detached reference
when restoration loses the race, and reload rq from the shared slot
before dereferencing it.

A KASAN-enabled i915 mock selftest reproduces the use-after-free before
this change and completes without a report after the fix.

Fixes: 4c71fd099513 ("drm/i915/gt: fix refcount underflow in intel_engine_park_heartbeat")
Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
Cc: <stable@vger.kernel.org> # v7.0+
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
Link: https://patch.msgid.link/20260808184302.1509585-1-shuangpeng.kernel@gmail.com
---
Hi,

I'm resending Shuangpeng's patch[*] so that it's tested by CI

Andi

https://patchwork.freedesktop.org/patch/745366/?series=171942&rev=1

- Changelog:
v2: Use the correct fix tag

 .../gpu/drm/i915/gt/intel_engine_heartbeat.c  | 22 ++++++++++++++-----
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
index 6424ecce8bcb..fc6c53c93cf5 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
@@ -81,10 +81,18 @@ heartbeat_create(struct intel_context *ce, gfp_t gfp)
 
 static void idle_pulse(struct intel_engine_cs *engine, struct i915_request *rq)
 {
+	struct i915_request *systole;
+
 	engine->wakeref_serial = READ_ONCE(engine->serial) + 1;
 	i915_request_add_active_barriers(rq);
-	if (!engine->heartbeat.systole && intel_engine_has_heartbeat(engine))
-		engine->heartbeat.systole = i915_request_get(rq);
+	if (READ_ONCE(engine->heartbeat.systole) ||
+	    !intel_engine_has_heartbeat(engine))
+		return;
+
+	systole = i915_request_get(rq);
+	/* The worker may have restored its detached systole in the meantime. */
+	if (cmpxchg(&engine->heartbeat.systole, NULL, systole))
+		i915_request_put(systole);
 }
 
 static void heartbeat_commit(struct i915_request *rq,
@@ -152,9 +160,11 @@ static void heartbeat(struct work_struct *wrk)
 	if (rq) {
 		if (i915_request_completed(rq))
 			i915_request_put(rq);
-		else
-			engine->heartbeat.systole = rq;
+		/* Keep a newer pulse that raced with the detached systole. */
+		else if (cmpxchg(&engine->heartbeat.systole, NULL, rq))
+			i915_request_put(rq);
 	}
+	rq = READ_ONCE(engine->heartbeat.systole);
 
 	if (!intel_engine_pm_get_if_awake(engine))
 		return;
@@ -163,11 +173,11 @@ static void heartbeat(struct work_struct *wrk)
 		goto out;
 
 	if (i915_sched_engine_disabled(engine->sched_engine)) {
-		reset_engine(engine, engine->heartbeat.systole);
+		reset_engine(engine, rq);
 		goto out;
 	}
 
-	if (engine->heartbeat.systole) {
+	if (rq) {
 		long delay = READ_ONCE(engine->props.heartbeat_interval_ms);
 
 		/* Safeguard against too-fast worker invocations */
-- 
2.55.0

Re: [PATCH v2] drm/i915/gt: Fix request use-after-free in heartbeat()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] heartbeat() dereferences the local pointer `rq` without holding a strong reference, making it vulnerable to a Use-After-Free if intel_engine_park_heartbeat() concurrently clears the shared pointer.
--

commit aa7ae2412e3834bb064c9442f0f96ffd0058a8ae
Author: Shuangpeng Bai <shuangpeng.kernel@gmail.com>

drm/i915/gt: Fix request use-after-free in heartbeat()

This commit addresses a request use-after-free in heartbeat() by using cmpxchg 
to manage the detached systole reference and replacing direct struct accesses 
with a local pointer.

Link: https://patch.msgid.link/20260808184302.1509585-1-shuangpeng.kernel@gmail.com

> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> index 6424ecce8bcbe..fc6c53c93cf57 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
[ ... ]
> @@ -152,9 +160,11 @@ static void heartbeat(struct work_struct *wrk)
>  	if (rq) {
>  		if (i915_request_completed(rq))
>  			i915_request_put(rq);
> -		else
> -			engine->heartbeat.systole = rq;
> +		/* Keep a newer pulse that raced with the detached systole. */
> +		else if (cmpxchg(&engine->heartbeat.systole, NULL, rq))
> +			i915_request_put(rq);
>  	}
> +	rq = READ_ONCE(engine->heartbeat.systole);

[Severity: High]
This is a pre-existing issue, but does heartbeat() safely dereference rq
without holding a strong reference?

If userspace toggles sysfs heartbeat intervals, intel_engine_park_heartbeat()
can execute concurrently, clearing the shared pointer and dropping the
reference:

intel_engine_park_heartbeat() {
    ...
    rq = xchg(&engine->heartbeat.systole, NULL);
    if (rq)
        i915_request_put(rq);
    ...
}

Since rq is loaded locklessly here without a reference bump, could this
result in a Use-After-Free later in heartbeat() when accessing
rq->emitted_jiffies?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830225708.3959691-1-andi.shyti@linux.intel.com?part=1