Re: [PATCH v6 05/18] platform/x86/intel/pmt: Do not remap when using callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] NULL pointer dereference in `pmt_telem_read32()` for callback-managed endpoints.
- [High] Resource access control bypass via sysfs `mmap` for callback-managed endpoints.
- [Medium] Application Energy Telemetry (AET) integration silently fails for callback-managed endpoints.
--

commit 4b5eaf240c4891c9b29025f06df395bf25b62269
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

platform/x86/intel/pmt: Do not remap when using callbacks

A callback indicates that the parent driver is responsible for accessing
the data area. Creating a PMT memory remap is redundant. If a read_telem
callback has been provided, do not create a remap.

> diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
> index 402d51df834a6..1c7a372b4fc42 100644
> --- a/drivers/platform/x86/intel/pmt/class.c
> +++ b/drivers/platform/x86/intel/pmt/class.c
[ ... ]
> @@ -403,14 +403,19 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
>  	if (!entry->size)
>  		return 0;
>  
> -	res.start = entry->base_addr;
> -	res.end = res.start + entry->size - 1;
> -	res.flags = IORESOURCE_MEM;
> +	/*
> +	 * The read_telem callback is responsible for this mapping, and may have
> +	 * different requirements for use. If the callback is present do not
> +	 * create the map.
> +	 */
> +	if (!(entry->cb && entry->cb->read_telem)) {
> +		struct resource res = DEFINE_RES_MEM(entry->base_addr, entry->size);
>  
> -	entry->base = devm_ioremap_resource(dev, &res);
> -	if (IS_ERR(entry->base)) {
> -		ret = PTR_ERR(entry->base);
> -		goto fail_ioremap;
> +		entry->base = devm_ioremap_resource(dev, &res);
> +		if (IS_ERR(entry->base)) {
> +			ret = PTR_ERR(entry->base);
> +			goto fail_ioremap;
> +		}
>  	}

[Severity: High]
Does leaving entry->base unmapped cause a NULL pointer dereference in
pmt_telem_read32()?

Because entry->base is intentionally left unmapped (NULL) for callback-managed
devices, any invocation of pmt_telem_read32() on such a device will
unconditionally dereference ep->base:

drivers/platform/x86/intel/pmt/telemetry.c:pmt_telem_read32() {
    ...
    memcpy_fromio(data, ep->base + offset, NUM_BYTES_DWORD(count));
    ...
}

[Severity: High]
Does unconditionally registering intel_pmt_mmap() in this function allow
userspace to bypass the device-specific callback requirements?

If a callback is required for safe access, the mmap handler still directly maps
the physical address to userspace:

drivers/platform/x86/intel/pmt/class.c:intel_pmt_mmap() {
    ...
	if (io_remap_pfn_range(vma, vma->vm_start, pfn,
		vsize, vma->vm_page_prot))
		return -EAGAIN;
    ...
}

Because entry->pmt_bin_attr.mmap = intel_pmt_mmap; is unconditionally set a
few lines below, this exposes a lockless bypass to userspace for hardware that
explicitly requires callback synchronization.

[Severity: Medium]
Will this silently break Application Energy Telemetry (AET) integration for
callback-managed endpoints?

During runtime reads, the AET driver expects the region address to be mapped,
but since it is now NULL, it skips all regions without emitting errors:

arch/x86/kernel/cpu/resctrl/intel_aet.c:intel_aet_read_event() {
    ...
	for (int i = 0; i < e->pfg->count; i++) {
		if (!e->pfg->regions[i].addr)
			continue;
        ...
		evtcount = readq(e->pfg->regions[i].addr + idx * sizeof(u64));
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904182451.1164868-20-michael.j.ruhl@intel.com?part=5