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

[sashiko-bot@kernel.org]

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

New issues:
- [High] `pmt_telem_read32()` does not check for the `read_telem` callback and unconditionally dereferences `ep->base`, which the target patch now leaves as NULL for callback-driven devices.

Pre-existing issues:
- [High] The sysfs `mmap` interface (`intel_pmt_mmap`) unconditionally maps physical memory directly to userspace, completely bypassing the `read_telem` callback required for safe hardware access and power management.
--

commit a61b8465842029c0f2ced5b0fa7ec2cd402fcbfa
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]
Since this change leaves entry->base as NULL when a callback is present,
will calling the exported pmt_telem_read32() API on a callback-driven
endpoint result in a NULL pointer dereference?

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

The function unconditionally dereferences ep->base without checking if a
read_telem callback should be used instead.


>  
>  	sysfs_bin_attr_init(&entry->pmt_bin_attr);

[Severity: High]
This is a pre-existing issue, but does unconditionally exposing the mmap
handler to userspace allow users to circumvent the hardware state management
that the read_telem callback is meant to provide?

In drivers/platform/x86/intel/pmt/class.c:intel_pmt_dev_register():
    ...
    entry->pmt_bin_attr.mmap = intel_pmt_mmap;
    ...

Because intel_pmt_mmap() maps physical memory directly to userspace:

In 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))
    ...

A user can read from the mapped memory region directly. Could this bypass
locking and power management (like pm_runtime_get_sync), leading to hangs
or Machine Check Exceptions if userspace accesses PCIe MMIO space while the
device is in D3?

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