Re: [PATCH v5 1/4] drm/xe/forcewake: synchronize engine-cycle access with unplug

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

Pre-existing issues:
- [High] Incomplete device unplug synchronization in query ioctls leading to potential MMIO access after unplug.
- [Medium] Missing array_index_nospec() on user-provided indices creates Spectre v1 gadgets.
--

--- Patch [1]: [PATCH v5 1/4] drm/xe/forcewake: synchronize engine-cycle access with unplug ---
commit cd6f1d67df5ef0b65b3a0c38b2fc6c7bd6e42c60
Author: Zongyao Bai <[email protected]>

drm/xe/forcewake: synchronize engine-cycle access with unplug

This patch adds drm_dev_enter() and drm_dev_exit() to query_engine_cycles()
to protect forcewake and MMIO accesses against device unplug operations. It
also updates the probe failure path to use drm_dev_unplug() to ensure proper
synchronization.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 71111ad324658..81b31325e5817 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c

[ ... ]

> @@ -1166,7 +1166,7 @@ int xe_device_probe(struct xe_device *xe)
>  
>  err_unregister_display:
>  	xe_display_unregister(xe);
> -	drm_dev_unregister(&xe->drm);
> +	drm_dev_unplug(&xe->drm);
>  
>  	return err;
>  }
> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> index dc975f5953683..3c356bc48e2ab 100644
> --- a/drivers/gpu/drm/xe/xe_query.c
> +++ b/drivers/gpu/drm/xe/xe_query.c

[ ... ]

> @@ -143,28 +145,44 @@ query_engine_cycles(struct xe_device *xe,
>  		return -EINVAL;
>  
>  	eci = &resp.eci;
> +	if (!drm_dev_enter(&xe->drm, &dev_idx))
> +		return -ENODEV;
> +

[Severity: High]
This is a pre-existing issue, but since this patch introduces device unplug
synchronization for query_engine_cycles(), should similar protection be added
to other queries like query_pxp_status() and query_oa_units()?

In xe_query.c, query_pxp_status() accesses hardware status without SRCU
protection:

query_pxp_status()
    xe_pxp_get_readiness_status(xe->pxp);

Similarly, query_oa_units() reads MMIO indirectly:

query_oa_units()
    xe_oa_timestamp_frequency()
        xe_mmio_read32()

Without drm_dev_enter() protection, can these queries cause page faults if a
userspace process calls DRM_IOCTL_XE_DEVICE_QUERY concurrently with a device
unplug event?

>  	gt = xe_device_get_gt(xe, eci->gt_id);
> -	if (!gt)
> -		return -EINVAL;
> +	if (!gt) {
> +		err = -EINVAL;
> +		goto out;
> +	}
>  
> -	if (eci->engine_class >= ARRAY_SIZE(user_to_xe_engine_class))
> -		return -EINVAL;
> +	if (eci->engine_class >= ARRAY_SIZE(user_to_xe_engine_class)) {
> +		err = -EINVAL;
> +		goto out;
> +	}
>  
>  	hwe = xe_gt_hw_engine(gt, user_to_xe_engine_class[eci->engine_class],
>  			      eci->engine_instance, true);

[Severity: Medium]
This is a pre-existing issue, but does the engine_class array access need
mitigation against speculative execution?

The eci->engine_class index comes directly from userspace and is used
to access the user_to_xe_engine_class array after a bounds check.
Could this be used to bypass bounds checks speculatively and leak
information via cache side channels?

Additionally, earlier in this function xe_device_get_gt() is called with
eci->gt_id, which also originates from userspace:

xe_device_get_gt()
    if (gt_id >= xe->info.tile_count * xe->info.max_gt_per_tile)
        return NULL;
    tile = &xe->tiles[gt_id / xe->info.max_gt_per_tile];

Should array_index_nospec() be used for these userspace-provided indices?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1