Re: [PATCH v7 13/20] drm/xe/vsec: Add base_offset to allow for more flexibilty

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Time-of-check to time-of-use (TOCTOU) race condition in xe_pmt_telem_read when calculating telem_addr.
--

commit 7e5dc04a0fcd634a62684607ee28e4cb12e99eb2
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Add base_offset to allow for more flexibilty

This commit introduces a base_offset for pmt_telem_read() to support other
devices that require a non-fixed offset. It adds a check to verify that the
offset has been set before allowing device access.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index ca8414441a692..856973473fd75 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
[ ... ]
> @@ -173,14 +173,15 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
>   * Return:
>   * * -ENODEV device is not available (hotplug)
>   * * -EINVAL for VF and PMT WATCHER requests
> + * * -EAGAIN base_offset has not been set, so init path has not yet been done.
>   * * count   number of bytes read on success
>   */
>  int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset,
>  		      u32 count)
>  {
>  	struct xe_device *xe = kdev_to_xe_device(dev);
>  	u32 cap_type = FIELD_GET(GUID_CAP_TYPE, guid);
> +	void __iomem *telem_addr = xe->mmio.regs + xe->pmt.base_offset;

[Severity: High]
Can this cause an out-of-bounds MMIO read due to a time-of-check to
time-of-use (TOCTOU) race?

If an unprivileged user reads the hwmon sysfs files (e.g., energy1_input)
during driver load, xe_pmt_telem_read() can be invoked concurrently with
xe_device_probe() before xe_vsec_init() sets base_offset.

At this point, xe->pmt.base_offset is 0, so telem_addr is evaluated as
xe->mmio.regs.

>  	u32 mem_region;
>  	u32 offset;
>  	int ret = 0;
> @@ -194,6 +195,11 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  		goto dev_exit;
>  	}
>  
> +	if (!xe->pmt.base_offset) {
> +		ret = -EAGAIN;
> +		goto dev_exit;
> +	}

[Severity: High]
Because drm_dev_enter() (called just before this in the function) acts as a
compiler and memory barrier, if xe_vsec_init() executes concurrently and
sets base_offset:

CPU1
xe_vsec_init()
   xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;

CPU2
xe_pmt_telem_read()
   // calculated telem_addr earlier with base_offset == 0
   if (!xe->pmt.base_offset) // Now reads BMG_TELEMETRY_OFFSET, passes

Will the function then proceed to use the stale telem_addr (derived from 0)
instead of the proper offset, resulting in reading from the wrong hardware
registers when memcpy_fromio() is later called?

Should telem_addr be computed after this validation check to ensure the
correct offset is used?

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