Re: [PATCH v2 05/10] drm/xe/vsec: Support possible hotplug exit

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

Pre-existing issues:
- [High] The `xe_pmt_telem_read` function performs a `memcpy_fromio` without validating that the requested telemetry read fits within the `xe->mmio.regs` mapped region. The `user_offset` and `count` parameters passed from the sysfs interface are bounded by `entry->size` (derived from the device's PMT discovery header). However, the hardware-provided size is implicitly trusted and never validated against the driver's actual MMIO mapping limits. A malicious or buggy hardware device could advertise an artificially large `header->size`, bypassing the `sysfs` offset limits. Because `xe_pmt_telem_read` calculates `telem_addr = xe->mmio.regs + offset + user_offset` and performs `memcpy_fromio`, this permits an out-of-bounds read past the end of the `xe->mmio.regs` mapping. Reading arbitrary unmapped physical memory or unrelated device registers could cause a system crash, leak sensitive hardware state, or trigger unintended side effects from read-sensitive registers.
--

--- Patch [5]: [PATCH v2 05/10] drm/xe/vsec: Support possible hotplug exit ---
commit 240693c221b75275ccbcc4f83244797a5eb5edf9
Author: Michael J. Ruhl <[email protected]>

drm/xe/vsec: Support possible hotplug exit

This patch adds drm_dev_enter() and drm_dev_exit() to xe_pmt_telem_read()
to verify the device is valid in the hotplug context before accessing it. It
also updates error handling to properly exit the DRM device context on failure.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
[ ... ]
> @@ -148,14 +155,20 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  	u32 cap_type = FIELD_GET(GUID_CAP_TYPE, guid);
>  	u32 mem_region;
>  	u32 offset;
> -	int ret;
> +	int ret = 0;
> +	int idx;
>  
> -	if (!xe->soc_remapper.set_telem_region)
> +	if (!drm_dev_enter(&xe->drm, &idx))
>  		return -ENODEV;
>  
> +	if (!xe->soc_remapper.set_telem_region) {
> +		ret = -EINVAL;
> +		goto dev_exit;
> +	}
> +
>  	ret = xe_guid_decode(guid, &mem_region, &offset);
>  	if (ret)
> -		return ret;
> +		goto dev_exit;
>  
>  	telem_addr += offset + user_offset;

[Severity: High]
This is a pre-existing issue, but does this code need to validate that the
requested telemetry read fits within the xe->mmio.size limit?

If a device provides a malicious or buggy PMT discovery header that advertises a
large entry->size, the sysfs offset limits would allow large user_offset and
count values.

Could the subsequent memcpy_fromio(data, telem_addr, count) further down in this
function then read past the end of the xe->mmio.regs mapping, potentially
causing a crash or leaking state from unmapped or read-sensitive registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5