Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] g2h_context_lookup() returns a context pointer without holding a reference or RCU lock, making it vulnerable to concurrent destruction and use-after-free for unsolicited messages. - [High] Race condition between setting `context_destroyed` and sending the deregister message allows a compromised GuC to trigger a logical Use-After-Free. -- commit cb6dcb7e78a9d3169d35435ce55f5ecfc640397c Author: Umesh Nerlige Ramappa <[email protected]> drm/i915/guc: Reject unsolicited deregister-done G2H intel_guc_deregister_done_process_msg() takes the ctx_id straight out of the G2H payload, resolves it via g2h_context_lookup() and then calls decr_outstanding_submission_g2h() unconditionally. Ensure that we are expecting a DEREGISTER_DONE G2H for that context before changing outstanding_submission state. Return EPROTO on error. > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index 9d3bd16adf406..85fbef30dbecb 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -5065,6 +5065,18 @@ int intel_guc_deregister_done_process_msg(struct > intel_guc *guc, > if (unlikely(!ce)) > return -EPROTO; > > + /* > + * A deregister H2G is only ever sent for a context that has been put in > + * one of these two states first, so anything else means this G2H does > + * not correspond to an outstanding request of ours. > + */ > + if (unlikely(!context_wait_for_deregister_to_register(ce) && [Severity: High] This is a pre-existing issue, but this patch adds new logic directly on the vulnerable path. Can this lead to a use-after-free if the context is concurrently destroyed? Earlier in intel_guc_deregister_done_process_msg() in drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c, g2h_context_lookup() returns a context pointer ce without holding a reference or an RCU lock. If submission_disabled(guc) becomes true concurrently, another thread dropping the final reference to ce might immediately call __guc_context_destroy(ce), freeing the memory. Will checking ce->guc_state.sched_state in the newly added check dereference a potentially freed pointer? > + !context_destroyed(ce))) { [Severity: High] This isn't a bug introduced by this patch, but the newly introduced check fails to protect against a race condition that can cause a use-after-free. In guc_lrc_desc_unpin() in drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c, a context is marked as destroyed before the deregister message is sent: if (likely(!disabled)) { __intel_gt_pm_get(gt); set_context_destroyed(ce); clr_context_registered(ce); } spin_unlock_irqrestore(&ce->guc_state.lock, flags); if (unlikely(disabled)) { ... } ret = deregister_context(ce, ce->guc_id.id); If a spurious DEREGISTER_DONE G2H message is processed by the CT worker thread in the window between unlocking the spinlock and calling deregister_context(), the new check context_destroyed(ce) will evaluate to true, allowing the message to be processed. Will this cause the CT thread to proceed and call __guc_context_destroy(ce) to free the memory, resulting in a use-after-free when deregister_context() is subsequently called? > + guc_err(guc, "Unexpected deregister done, ctx_id %u sched_state > 0x%x\n", > + ctx_id, ce->guc_state.sched_state); > + return -EPROTO; > + } > + > trace_intel_context_deregister_done(ce); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
