Re: [PATCH] drm/xe: Add wa_14028434957 to Graphics Version 35.11

["Gupta, Varun" <varun.gupta@intel.com>]

On 09-Oct-26 12:43 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] drm: missing error return after drmm_add_action_or_reset() failure leads to leaked work and UAF
> - [High] xe: Use-after-free/MMIO fault due to deferred workqueue cancellation in drmm
> - [High] xe: nonfatal_msi_work accesses MMIO without waking device, causing suspended MMIO accesses
> - [High] drm/xe: UAF and uninitialized work_struct in csc_hw_error_work
> --
>
> commit c1e7bdf12138e40e4c7ecaaa2d05a9ee296aa085
> Author: Varun Gupta <varun.gupta@intel.com>
> Subject: drm/xe: Add wa_14028434957 to Graphics Version 35.11
>
> This patch bypasses a faulty AER path by intercepting uncorrectable
> non-fatal MSI errors and routing them to a deferred workqueue. The new
> workqueue reuses the GFSP error containment logic and wedges the device
> if a reset is required.
>
>> diff --git a/drivers/gpu/drm/xe/xe_hw_error.c b/drivers/gpu/drm/xe/xe_hw_error.c
>> index 5f2abc9485ff5..847ca1ae411ef 100644
>> --- a/drivers/gpu/drm/xe/xe_hw_error.c
>> +++ b/drivers/gpu/drm/xe/xe_hw_error.c
> [ ... ]
>> @@ -173,6 +177,27 @@ static void csc_hw_error_work(struct work_struct *work)
>>   	xe_survivability_mode_runtime_enable(xe);
>>   }
>>   
>> +static void nonfatal_msi_work(struct work_struct *work)
>> +{
>> +	struct xe_drm_ras *ras = container_of(work, typeof(*ras), nonfatal_msi_work);
>> +	struct xe_device *xe = container_of(ras, typeof(*xe), ras);
>> +	enum xe_ras_recovery_action action = xe_ras_process_errors(xe);
> [Severity: High]
> Does this workqueue execution need to resume the device first?
>
> Because this runs asynchronously in a worker thread, the runtime PM
> reference from the interrupt might already be released, allowing the device
> to auto-suspend to D3. The nested call to xe_sysctrl_send_command() uses
> guard(xe_pm_runtime_noresume)(xe), which blocks future suspends but does
> not wake a device that is already suspended. This means MMIO accesses
> to SYSCTRL_MB_CTRL might target suspended hardware.
>
> Other workers like xe_sysctrl_work() seem to use guard(xe_pm_runtime)(xe)
> to explicitly wake the device.
Noted, added PM guard in v2.
>> +
>> +	/*
>> +	 * TODO: Request an SBR instead of wedging, once VR driver
>> +	 * quiescing support is in place.
>> +	 */
>> +	if (action != XE_RAS_RECOVERY_ACTION_RECOVERED)
>> +		xe_device_declare_wedged(xe);
>> +}
>> +
>> +static void hw_error_fini(struct drm_device *drm, void *arg)
>> +{
>> +	struct xe_device *xe = arg;
>> +
>> +	cancel_work_sync(&xe->ras.nonfatal_msi_work);
> [Severity: High]
> Should this function also cancel csc_hw_error_work?
>
> The code initializes csc_hw_error_work in xe_hw_error_init(), but it
> doesn't appear to be cancelled during teardown. With a fini function now
> added, leaving csc_hw_error_work uncancelled might allow it to run after
> the xe_device structures are freed.
>
> Also, it looks like csc_hw_error_work is only initialized for the root tile
> in xe_hw_error_init():
>
>      struct xe_tile *tile = xe_device_get_root_tile(xe);
>      ...
>      INIT_WORK(&tile->csc_hw_error_work, csc_hw_error_work);
>
> If a non-root tile receives a hardware error and schedules this work, might
> it operate on an uninitialized work_struct?
This looks like a pre-existing thing out of scope for this patch, will 
spin another patch if required.
>> +}
>> +
> [ ... ]
>> @@ -436,6 +461,15 @@ static void hw_error_source_handler(struct xe_tile *tile, const enum hardware_er
>>   	 * here for those platforms.
>>   	 */
>>   	if (xe->info.has_sysctrl) {
>> +		/*
>> +		 * Defer non-fatal MSI handling to process context because the GFSP
>> +		 * query can sleep.
>> +		 */
>> +		if (hw_err == HARDWARE_ERROR_NONFATAL && XE_DEVICE_WA(xe, 14028434957)) {
>> +			schedule_work(&xe->ras.nonfatal_msi_work);
> [Severity: High]
> Could this work execute after the MMIO mappings have been unmapped?
>
> Since hw_error_fini() is registered as a drmm action, its execution is
> deferred until the last DRM reference is dropped (like when userspace
> closes the device file). However, devres actions unmap the device's MMIO
> mappings synchronously during PCI unbind.
>
> If this work is queued just before the IRQ is torn down and executes after
> the unbind finishes, the nested xe_ras_process_errors() could attempt to
> access unmapped MMIO regions via xe_sysctrl_send_command(), resulting in a
> fatal page fault.
Noted, made needed change in v2.
>> +			return;
>> +		}
>> +
>>   		drm_err_ratelimited(&xe->drm, HW_ERR "Invalid error routing\n");
>>   		return;
>>   	}
>> @@ -546,6 +580,11 @@ void xe_hw_error_init(struct xe_device *xe)
>>   {
>>   	struct xe_tile *tile = xe_device_get_root_tile(xe);
>>   
>> +	INIT_WORK(&xe->ras.nonfatal_msi_work, nonfatal_msi_work);
>> +
>> +	if (drmm_add_action_or_reset(&xe->drm, hw_error_fini, xe))
>> +		drm_err(&xe->drm, "Failed to register hw_error_fini\n");
> [Severity: High]
> Is a return statement missing here?
>
> If drmm_add_action_or_reset() fails, it logs an error but initialization
> continues. This means the driver can be fully initialized without
> hw_error_fini() being registered. If nonfatal_msi_work gets scheduled,
> it will never be cancelled on teardown, potentially leading to a
> use-after-free regression.

Thought about doing this in v1 itself but went against it to avoid 
changing return types of functions,
but looks like it is really needed to prevent UAF so implementing this 
in v2.

Thanks

>> +
>>   	if (!IS_DGFX(xe) || IS_SRIOV_VF(xe))
>>   		return;
>>