Re: [PATCH v1] drm/xe/i2c: Disable IRQ on unbind

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch attempts to fix a Use-After-Free by disabling the I2C IRQ in hardware during `xe_i2c_remove()`, but it leaves multiple UAF vectors open: a synchronous UAF in the later devres teardown path due to a dangling `xe->i2c` pointer, and asynchronous UAFs due to missing IRQ synchronization and work cancellation.
--

commit 45a43617b8b535b9ff0b0cfa6ca5c0e40ebb1dc5
Author: Raag Jadav <raag.jadav@intel.com>

drm/xe/i2c: Disable IRQ on unbind

This commit disables the I2C IRQ explicitly during driver unbind. It
attempts to fix a potential use-after-free if the SGUnit IRQ is hit in the
small window before the IRQ is disabled by the subsystem.

> diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
> index d8fa68206f418..82b22c5412bab 100644
> --- a/drivers/gpu/drm/xe/xe_i2c.c
> +++ b/drivers/gpu/drm/xe/xe_i2c.c
> @@ -324,8 +324,10 @@ void xe_i2c_pm_resume(struct xe_device *xe, bool d3cold)
>  static void xe_i2c_remove(void *data)
>  {
>  	struct xe_i2c *i2c = data;
> +	struct xe_device *xe = tile_to_xe(i2c->mmio->tile);
>  	unsigned int i;
>  
> +	xe_i2c_irq_reset(xe);
>  	xe_amc_exit(i2c);

[Severity: High]
This is a pre-existing issue, but does this teardown sequence leave open
a synchronous use-after-free in the devres teardown path?

During driver unbind, devres executes teardown actions in LIFO order. While
xe_i2c_remove() is called here, it fails to set xe->i2c = NULL. The memory
for the i2c struct (allocated via devm_kzalloc()) is then freed by devres.

Later in the teardown, the irq_uninstall() devres action executes, leading
to this call chain:

irq_uninstall() -> xe_irq_reset() -> xe_i2c_irq_reset() -> xe_i2c_irq_present()

Inside xe_i2c_irq_present(), the dangling pointer is dereferenced:

static bool xe_i2c_irq_present(struct xe_device *xe)
{
	return xe->i2c && xe->i2c->ep.capabilities & XE_I2C_EP_CAP_IRQ &&
		!xe_survivability_mode_is_boot_enabled(xe);
}

Could we prevent this by clearing the pointer here with xe->i2c = NULL?

Additionally, is there an asynchronous use-after-free if a concurrent IRQ
handler is executing? Without a call to synchronize_irq(), a concurrent
handler could execute immediately after xe_amc_exit() frees i2c->amc, and
attempt to queue work on the freed memory:

void xe_amc_handle_alert(struct xe_i2c *i2c)
{
	queue_work(system_long_wq, &i2c->amc->work);
}

Would it be appropriate to include synchronize_irq() and perhaps a call to
cancel_work_sync() in this teardown path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911044021.2393342-1-raag.jadav@intel.com?part=1