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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] core: async devm registration during unbind causes resource leak and UAF
- [Low] style: mixing goto-based cleanup labels with scoped_guard in same function
--

commit 07a655e6731c9aa568ad5d0e90b50bcc5c2bdb0a
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.
    
    Update 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
> @@ -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)) {
> +		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);
> @@ -350,6 +407,7 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  		memcpy_fromio(data, telem_addr, count);
>  	}
>  
> +runtime_exit:
>  	xe_pm_runtime_put(xe);
>  
>  dev_exit:

[Severity: Low]
Does this code mix goto-based cleanup labels with scoped_guard in the same
function?

The cleanup subsystem guidelines explicitly state that functions should not
mix goto-based cleanup labels with scope-based cleanup declarations like
scoped_guard.

> @@ -397,6 +455,10 @@ static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
>  	disc_addr += inst + offset;
>  
>  	xe_pm_runtime_get(xe);
> +	if (!wait_for_fw(xe)) {
> +		ret = -ENODATA;
> +		goto runtime_exit;
> +	}
>  
>  	scoped_guard(mutex, &xe->pmt.lock) {
>  		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> @@ -404,6 +466,7 @@ static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
>  		*reg = readl(disc_addr);
>  	}
>  
> +runtime_exit:
>  	xe_pm_runtime_put(xe);
>  
>  dev_exit:

[Severity: Low]
Is it intentional to mix goto labels and scoped_guard here as well?

Using goto-based cleanup labels together with scoped_guard in the same
function is discouraged by the cleanup subsystem guidelines.

> @@ -434,6 +497,10 @@ static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
>  	disc_addr += inst + offset;
>  
>  	xe_pm_runtime_get(xe);
> +	if (!wait_for_fw(xe)) {
> +		ret = -ENODATA;
> +		goto runtime_exit;
> +	}
>  
>  	scoped_guard(mutex, &xe->pmt.lock) {
>  		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> @@ -441,6 +508,7 @@ static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
>  		writel(reg, disc_addr);
>  	}
>  
> +runtime_exit:
>  	xe_pm_runtime_put(xe);
>  
>  dev_exit:

[Severity: Low]
Does this function also mix goto-based cleanup labels with scoped_guard?

The same cleanup subsystem guidelines apply here, which advise against mixing
goto-based cleanup labels and scope-based cleanups in a single function.

> @@ -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);
> +}

[ ... ]

> @@ -504,12 +606,26 @@ void xe_vsec_init(struct xe_device *xe)
>  
>  	case XE_VSEC_CRI:
>  		xe->pmt.base_offset = CRI_PMT_OFFSET;
> +
> +		xe->pmt.retry_count = 0;
> +		INIT_DELAYED_WORK(&xe->pmt.work, cri_late_bind_probe_work);
> +
> +		xe_pm_runtime_get_noresume(xe);
> +		if (!xe_sysctrl_is_oobmsm_fw_ready(xe)) {
> +			schedule_delayed_work(&xe->pmt.work,
> +					      msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC));
> +			return devm_add_action_or_reset(xe->drm.dev,
> +							vsec_disable_late_bind_work,
> +							xe);
> +		}
> +

[Severity: High]
Does this setup cause an async devm registration during unbind, leading to a
resource leak and Use-After-Free?

During driver unbind, devres_release_all() splices the dev->devres_head list
to a local todo list. It then executes vsec_disable_late_bind_work(), which
waits for the cri_late_bind_probe_work() thread to complete.

Concurrently, that worker thread calls cri_late_bind_probe(), which calls
intel_vsec_register(), adding a new devm action to the now-empty
dev->devres_head list.

Once the worker completes, vsec_disable_late_bind_work() unblocks and
devres_release_all() finishes its local list. The newly added devm action on
dev->devres_head is never checked or executed, leaving the auxiliary device
leaked and its sysfs nodes pointing to a freed parent.

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