Re: [PATCH v11 17/20] drm/xe/vsec: Support late bind fw information

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] core: devres leak and UAF due to concurrent delayed work
- [High] pm: invalid hardware access during suspend from non-freezable workqueue
- [Medium] locking: SRCU read lock held across 20s msleep blocks hotunplug
- [Medium] locking: lock inversion between pmt.lock and device core locks
- [Medium] task: 20s uninterruptible msleep enables local DoS
--

commit 0381c7581cc6bd07b52526c81bae84b56ea49be4
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Support late bind fw information

CRI FW is loaded on power on.  Because of this, access to the FW cannot
be done until it is running. This commit updates the XE PMT probe and
access to check for late bind devices, verify, and wait for the
appropriate FW state before probe or access.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 0f6e3be520c09..5384fef5e5ac5 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
[ ... ]
> @@ -281,6 +291,48 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
>  	return -ENODEV;
>  }
>  
> +static void cri_late_bind_probe_work(struct work_struct *work)
> +{
> +	struct xe_device *xe = container_of(work, struct xe_device, pmt.work.work);
> +
> +	if (xe_sysctrl_is_oobmsm_fw_ready(xe)) {

[Severity: High]
Can this result in hardware hangs or master aborts during system suspend?

This delayed work is queued on system_wq, which lacks the WQ_FREEZABLE flag.
Because the driver's system suspend path does not cancel or flush this work,
the worker thread could wake up during or after suspend.

If that happens, xe_sysctrl_is_oobmsm_fw_ready() will attempt to read MMIO
registers while the PCI device is powered down in D3cold.

> +		cri_late_bind_probe(xe);
> +		xe_pm_runtime_put(xe);
> +		return;
> +	}
> +
> +	xe->pmt.retry_count++;
> +
> +	/* wait up to 20 seconds */
> +	if (xe->pmt.retry_count == VSEC_LATE_BIND_RETRY) {
> +		drm_warn(&xe->drm, "PMT probe: Late Binding failed to complete\n");
> +		xe_pm_runtime_put(xe);
> +		return;
> +	}
> +
> +	if (!schedule_delayed_work(&xe->pmt.work, msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC)))
> +		xe_pm_runtime_put(xe);
> +}
> +
> +static bool wait_for_fw(struct xe_device *xe)
> +{
> +	int retries = VSEC_LATE_BIND_RETRY;  /* wait up to 20 secs */
> +
> +	if (xe->info.platform != XE_CRESCENTISLAND)
> +		return true;
> +
> +	while (retries--) {
> +		if (xe_sysctrl_is_oobmsm_fw_ready(xe))
> +			return true;
> +
> +		msleep(VSEC_LATE_BIND_DELAY_MSEC);

[Severity: Medium]
Could this uninterruptible sleep cause localized task lockups?

If user-space initiates a read on PMT telemetry and the firmware never
completes, this will loop on msleep() for up to 20 seconds. The thread
will be blocked in uninterruptible sleep (TASK_UNINTERRUPTIBLE) and will
ignore all signals, including SIGKILL, until the timeout expires.

Could this be exploited by a local user to intentionally block multiple
threads, leading to inflated load averages or resource exhaustion?

> +	}
> +
> +	drm_warn(&xe->drm, "Late Binding failed to complete\n");
> +
> +	return false;
> +}
> +
>  /**
>   * xe_pmt_telem_read - Given a device and a PMT GUID, read data into a buffer
>   * @dev: valid Xe device
[ ... ]
> @@ -343,6 +395,11 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  		goto dev_exit;
>  	}
>  
> +	if (!wait_for_fw(xe)) {

[Severity: Medium]
Does holding the DRM SRCU read lock across this 20-second wait block
device hot-unplug?

User-space reads to PMT telemetry enter xe_pmt_telem_read(), which calls
drm_dev_enter() and acquires the SRCU read lock before reaching here.

If a device hot-unplug occurs while wait_for_fw() is sleeping for 20
seconds, drm_dev_unplug() will call synchronize_srcu(). This will block
the unplug process and critical kernel teardown paths until the timeout
expires and the SRCU reader exits.

> +		ret = -ENODATA;
> +		goto runtime_exit;
> +	}
> +
>  	scoped_guard(mutex, &xe->pmt.lock) {
>  		/* set SoC re-mapper index register based on GUID memory region */
>  		xe->soc_remapper.set_telem_region(xe, mem_region);
[ ... ]
> @@ -477,12 +545,46 @@ static enum xe_vsec get_platform_info(struct xe_device *xe)
>  	return vsec_platforms[xe->info.platform];
>  }
>  
> +static void cri_late_bind_probe(struct xe_device *xe)
> +{
> +	struct intel_vsec_platform_info *info;
> +	struct device *dev = xe->drm.dev;
> +	enum xe_vsec platform;
> +
> +	platform = get_platform_info(xe);
> +	if (platform != XE_VSEC_CRI)
> +		return;
> +
> +	info = &xe_vsec_info[platform];
> +	if (!info->headers)
> +		return;
> +
> +	guard(mutex)(&xe->pmt.lock);
> +
> +	info->priv_data = &xe_cri_pmt_cb;
> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +
> +	intel_vsec_register(dev, info);

[Severity: Medium]
Is there a lock inversion here between pmt.lock and the driver core locks?

This function acquires xe->pmt.lock via guard(mutex) and then calls
intel_vsec_register(), which interacts with the driver core to add
devices and trigger probes, acquiring driver core locks like device_lock.

When user-space accesses PMT sysfs files, the driver core acquires its
locks first (e.g., kernfs and device_lock), and then calls
xe_pmt_telem_read(), which acquires pmt.lock.

This circular dependency could lead to a lockdep splat or an AB-BA
self-deadlock.

> +}
> +
> +static void vsec_disable_late_bind_work(void *arg)
> +{
> +	struct xe_device *xe = arg;
> +
> +	/*
> +	 * If the queued work is canceled, the runtime reference needs to be
> +	 * released here.
> +	 */
> +	if (disable_delayed_work_sync(&xe->pmt.work))

[Severity: High]
Can this concurrent delayed work cause a devres leak and a Use-After-Free
of the parent device?

During device unbind, devres_release_all() splices the devres_head list
and iterates over it. It calls this devm action, which blocks here on
disable_delayed_work_sync().

Meanwhile, the delayed work can call cri_late_bind_probe(), which calls
intel_vsec_register(). This dynamically allocates and appends new devm
resources to the now-empty devres_head.

When the work finishes, devres_release_all() returns without checking
devres_head again. The new resources are permanently leaked. Also, because
the parent device is freed, the orphaned auxiliary devices will hold
dangling pointers to it.

> +		xe_pm_runtime_put(xe);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923181115.2514193-22-michael.j.ruhl@intel.com?part=17