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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Disabling I2C interrupts before flushing work or unregistering I2C clients causes subsequent I2C transfers to time out and fail.

Pre-existing issues:
- [High] The patch attempts to fix a Use-After-Free (UAF) by explicitly disabling the I2C IRQ in `xe_i2c_remove()`, but it fails to wait for already-executing IRQ handlers, leaving a critical race window open that leads to a UAF of `i2c->amc`.
- [High] Missing cancellation of `i2c->work` in `xe_i2c_remove()` leads to a Use-After-Free of the `i2c` structure.
- [High] Dangling `xe->i2c` pointer on `xe_i2c_probe()` error paths leads to a UAF during probe failure teardown.
--

commit 3fffddfb454e12e0901f531980c05e733fae1cc6
Author: Raag Jadav <raag.jadav@intel.com>

drm/xe/i2c: Disable IRQ on unbind

This commit attempts to fix a potential use-after-free in the unbind path by
explicitly disabling the I2C IRQ in xe_i2c_remove() before struct xe_i2c
is freed. It resets the I2C interrupt and clears the xe->i2c pointer.

> diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
> index d8fa68206f418..51ca9fb73b708 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);

[Severity: Medium]
Does disabling I2C interrupts before flushing work or unregistering clients
cause active I2C transfers to time out?

If xe_amc_work() is currently running and attempting an i2c_master_send(),
or if i2c_unregister_device() invokes client remove callbacks that perform
I2C transfers, the underlying designware I2C driver will wait for an interrupt
that never arrives. This could delay unbind by timing out and potentially
leave the hardware in an incorrect state.

>  	xe_amc_exit(i2c);

[Severity: High]
This is a pre-existing issue, but does xe_i2c_irq_reset() guarantee that an
already-executing IRQ handler has finished? 

Without a synchronization barrier like synchronize_irq(), CPU0 could be
handling an interrupt in xe_i2c_handle_smbus_alert(), while CPU1 executes
xe_i2c_remove() and calls xe_amc_exit(i2c) to free i2c->amc. 

If CPU0 then proceeds to evaluate the freed i2c->amc pointer:

xe_i2c_handle_smbus_alert(struct xe_i2c *i2c)
{
	...
	if (stat & DW_IC_SMBUS_INTR_ALERT && i2c->amc)
		xe_amc_handle_alert(i2c);
}

Could this still result in a use-after-free and memory corruption?

>  
>  	for (i = 0; i < XE_I2C_MAX_CLIENTS; i++) {
> @@ -335,6 +337,7 @@ static void xe_i2c_remove(void *data)
>  
>  	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
>  	xe_i2c_unregister_adapter(i2c);
> +	xe->i2c = NULL;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but should there be a call to
cancel_work_sync(&i2c->work) here to ensure any pending worker is stopped
before the memory is freed?

The notifier schedules i2c->work, but when xe_i2c_remove() unregisters
the notifier, it doesn't flush pending work. Since i2c is managed by devres
(devm_kzalloc), it will be freed shortly after xe_i2c_remove() returns. If
xe_i2c_client_work() executes after this, it will access the freed i2c
memory:

xe_i2c_client_work() {
	struct xe_i2c *i2c = container_of(work, struct xe_i2c, work);
	...
	i2c->client[XE_I2C_CLIENT_AMC] = i2c_new_client_device(...);
}

[Severity: High]
This is also a pre-existing issue, but in xe_i2c_probe(), the xe->i2c pointer
is assigned before initialization is complete:

xe_i2c_probe() {
	...
	xe->i2c = i2c;
	...
	ret = xe_i2c_register_adapter(i2c);
	if (ret) {
		bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
		return ret;
	}
	...
}

If xe_i2c_register_adapter() fails, multiple error paths return without
clearing xe->i2c. Since the IRQ handler is already active and the i2c memory
is managed by devm and will be freed, could an incoming hardware interrupt
trigger a use-after-free when xe_i2c_irq_handler() calls
xe_i2c_handle_smbus_alert(xe->i2c)?

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