Re: [PATCH v2 1/2] drm/xe/i2c: Expose AMC Alert reason sysfs

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The ABI documentation incorrectly states that the sysfs attribute is created when an alert is handled.
- [Medium] Lockless data race on `amc->alert_reason` between the workqueue writer and sysfs reader.

Pre-existing issues:
- [High] Use-after-free vulnerability during module teardown in `xe_amc_exit()` due to destroying `i2c->amc` while the I2C interrupt remains active.
- [High] Missing cancellation of `i2c->work` in `xe_i2c_remove()` leads to a Use-After-Free on driver unbind or error paths.
--

commit ae79b3f2589a0953606cef0104f63d635c6345fa
Author: Badal Nilawar <badal.nilawar@intel.com>

drm/xe/i2c: Expose AMC Alert reason sysfs

This commit exposes a read-only xe_amc_alert_reason sysfs attribute to
help users identify the required recovery action. It adds ABI documentation
and wires up the sysfs creation and AMC worker updates.

> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-amc b/Documentation/ABI/testing/sysfs-driver-intel-xe-amc
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-amc
> @@ -0,0 +1,25 @@
[ ... ]
> +		An AMC alert is delivered via an SMBUS interrupt and causes the
> +		device to be wedged, requiring vendor-specific recovery. This
> +		attribute is created when such an alert is handled and is
> +		available to all users as read-only.

[Severity: Medium]
The documentation states the attribute is created when an alert is handled,
but looking at xe_amc_init(), it appears to be created unconditionally
during driver probe. Could this confuse userspace tools waiting for the
file to appear as a signal that an alert occurred?

> diff --git a/drivers/gpu/drm/xe/xe_amc.c b/drivers/gpu/drm/xe/xe_amc.c
> index edd50bf8261ec..bb9d260f800ff 100644
> --- a/drivers/gpu/drm/xe/xe_amc.c
> +++ b/drivers/gpu/drm/xe/xe_amc.c
[ ... ]
> @@ -104,6 +109,42 @@ static const struct amc_request amc_get_alert_reason = {
[ ... ]
> +static ssize_t xe_amc_alert_reason_show(struct device *dev,
> +					struct device_attribute *attr, char *buf)
> +{
> +	struct xe_device *xe = pdev_to_xe_device(to_pci_dev(dev));
> +	struct xe_amc *amc = xe->i2c->amc;
> +
> +	return sysfs_emit(buf, "%s\n", amc_alert[amc->alert_reason]);
> +}

[Severity: Medium]
Is there a potential lockless data race here?

The worker thread in xe_amc_work() updates amc->alert_reason via normal
assignment, while a concurrent userspace read of the sysfs file can access
it here without synchronization or memory barriers (like READ_ONCE and
WRITE_ONCE).

[ ... ]
> @@ -176,6 +224,7 @@ void xe_amc_handle_alert(struct xe_i2c *i2c)
[ ... ]
>  void xe_amc_exit(struct xe_i2c *i2c)
>  {
>  	if (i2c->amc) {
> +		xe_amc_remove_alert_sysfs(i2c);
>  		cancel_work_sync(&i2c->amc->work);
>  		kfree(i2c->amc);
> +		i2c->amc = NULL;
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does freeing i2c->amc here
while the I2C hardware interrupt is still active leave us vulnerable to a
Use-After-Free?

The devm cleanup framework uninstalls the IRQ after removing the I2C
client. Concurrently, if a hardware alert fires exactly when the struct
is freed, could the interrupt handler read the dangling, non-NULL pointer
right before it is set to NULL?

drivers/gpu/drm/xe/xe_i2c.c:xe_i2c_handle_smbus_alert() {
    if (stat & DW_IC_SMBUS_INTR_ALERT && i2c->amc)
        xe_amc_handle_alert(i2c);
}

This could end up queuing work on the freed i2c->amc->work.

[Severity: High]
This is also a pre-existing issue, but I noticed another potential
Use-After-Free during teardown in xe_i2c_remove() 
(drivers/gpu/drm/xe/xe_i2c.c):

static void xe_i2c_remove(void *data)
{
    struct xe_i2c *i2c = data;
    unsigned int i;

    xe_amc_exit(i2c);
    ...
}

Since xe_i2c_notifier() schedules i2c->work, if xe_i2c_remove() completes
and devres frees the struct xe_i2c without first calling
cancel_work_sync(&i2c->work), what happens if the uncancelled work executes
afterwards?

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(i2c->adapter, &info);
}

Would this dereference the freed i2c structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910124151.3135801-4-badal.nilawar@intel.com?part=1