Re: [PATCH v7 1/3] drm/xe: Prevent VF memory IRQ from accessing a torn-down engine

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe: TOCTOU NULL pointer deref in query_engine_cycles() during unplug
--

commit f3434e1999c04ac6bc30530fad97ca5ad34cad2a
Author: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>

drm/xe: Prevent VF memory IRQ from accessing a torn-down engine

This commit invalidates the engine name during engine teardown in
hw_engine_fini() to prevent a NULL pointer dereference in IRQ handlers.
By clearing hwe->name, subsequent engine scans will skip the torn-down
engine.

> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 265f95c0c8993..55ee0c892c1e8 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -275,11 +275,22 @@ static const struct engine_info engine_infos[] = {
>  static void hw_engine_fini(void *arg)
>  {
>  	struct xe_hw_engine *hwe = arg;
> +	struct xe_gt *gt = hwe->gt;
> +
> +	if (gt) {
> +		struct xe_device *xe = gt_to_xe(gt);
> +		unsigned long flags;
>  
> -	if (hwe->exl_port)
> -		xe_execlist_port_destroy(hwe->exl_port);
> +		if (hwe->exl_port)
> +			xe_execlist_port_destroy(hwe->exl_port);
>  
> -	hwe->gt = NULL;
> +		spin_lock_irqsave(&xe->irq.lock, flags);
> +		hwe->name = NULL;
> +		hwe->gt = NULL;
> +		spin_unlock_irqrestore(&xe->irq.lock, flags);

[Severity: High]
Does this still leave a TOCTOU NULL pointer dereference in
query_engine_cycles() during device unplug?

Device removal (e.g., driver unbind or PCI unplug) invokes
xe_device_remove(), which calls drm_dev_unplug().

Since drm_dev_unplug() waits only for SRCU readers that called
drm_dev_enter(), and neither xe_drm_ioctl() nor query_engine_cycles()
uses drm_dev_enter(), the IOCTL can execute concurrently with the devres
teardown phase.

If hw_engine_fini() runs concurrently, it clears hwe->gt = NULL while
holding xe->irq.lock.

Concurrently, query_engine_cycles() can retrieve the engine via
xe_gt_hw_engine() just before hwe->name is cleared, and then call
hwe_read_timestamp():

drivers/gpu/drm/xe/xe_query.c:query_engine_cycles() {
    hwe = xe_gt_hw_engine(gt, user_to_xe_engine_class[eci->engine_class],
                          eci->engine_instance, true);
    if (!hwe)
        return -EINVAL;

    fw_domain = xe_hw_engine_to_fw_domain(hwe);

    xe_with_force_wake(fw_ref, gt_to_fw(gt), fw_domain) {
        if (!xe_force_wake_ref_has_domain(fw_ref.domains, fw_domain))
            return -EIO;

        hwe_read_timestamp(hwe, &resp.engine_cycles, &resp.cpu_timestamp,
                           &resp.cpu_delta, cpu_clock);
    }
}

Inside hwe_read_timestamp(), it dereferences hwe->gt without taking
xe->irq.lock:

drivers/gpu/drm/xe/xe_query.c:hwe_read_timestamp() {
    struct xe_mmio *mmio = &hwe->gt->mmio;
    ...
}

Can this lead to a NULL pointer dereference crash, allowing a local DoS by
triggering driver unbind while executing the query IOCTL?

> +	} else {
> +		hwe->name = NULL;
> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013152417.3189567-5-satyanarayana.k.v.p@intel.com?part=1