Re: [PATCH] drm/xe/guc: Handle GuC semaphore instruction field check error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe/guc: UAF in invalid semaphore handler due to lockless g2h lookup
--

commit cebb8bdaf89f1a8799fcb42b7d4d2d9171193941
Author: Jesus Narvaez <jesus.narvaez@intel.com>

drm/xe/guc: Handle GuC semaphore instruction field check error

Starting on NVL-P and newer platforms which support 64 bit semaphores,
this commit opts in to enable GuC to check semaphore instruction fields.
It adds a handler to deal with contexts marked as unrunnable if the
check fails.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -3708,6 +3708,53 @@ int xe_guc_exec_queue_cgp_sync_done_handler(struct xe_guc *guc, u32 *msg, u32 le
[ ... ]
> +int xe_guc_context_invalid_semaphore_instruction_field_handler(struct xe_guc *guc,
> +							       u32 *msg, u32 len)
> +{
[ ... ]
> +	q = g2h_exec_queue_lookup(guc, guc_id);
> +	if (unlikely(!q))
> +		return -EPROTO;
> +
> +	xe_gt_info(gt,
> +		   "GuC: invalid semaphore instruction field! guc_id=%d, type=%u lrca=0x%llx, token=0x%llx, data=0x%llx",
> +		   guc_id, type, lrc_address, token, data);
> +
> +	trace_xe_guc_invalid_semaphore_instruction_field(q);
> +
> +	/* Treat the same as engine reset */
> +	xe_guc_exec_queue_reset_trigger_cleanup(q);

[Severity: High]
Could this lead to a use-after-free during device unplug?

The handler looks up the execution queue using g2h_exec_queue_lookup(),
which fetches the pointer locklessly via xa_load() without acquiring a kref.

While the normal deregistration flow guarantees the queue exists until
processing DEREGISTER_CONTEXT_DONE, this is bypassed during device unplug
when drm_dev_enter() returns false. In the unplug scenario,
__guc_exec_queue_process_msg_cleanup directly queues asynchronous destruction
which frees the queue via kfree().

Because device teardown does not immediately stop the CT worker, G2H messages
can be processed concurrently with queue destruction. If the pointer is fetched
by xa_load() just before it is removed and freed, the handler will dereference
freed memory when calling the tracepoint and triggering cleanup.

Is there a need to validate the queue lifetime or acquire a reference here?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012192828.226700-2-jesus.narvaez@intel.com?part=1