Re: [PATCH v5 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:

New issues:
- [High] NULL pointer dereference in `pmt_telem_read32()` when accessing telemetry endpoints that provide a callback.
- [Medium] PMT regions using callbacks break the `telemetry_region` API contract, causing KUnit test failures and silently dropped telemetry data.

Pre-existing issues:
- [High] Userspace mmap unconditionally exposes telemetry memory, allowing it to bypass the `read_telem` callback and hardware protections.
--

commit 12a3129bb64bc1b8ec1d2e409cf11d07b5c7a14a
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>
Subject: platform/x86/intel/pmt: Do not remap when using callbacks

This commit updates the Intel PMT driver to skip creating a memory remap when
a read_telem callback is provided. It avoids redundant mappings for devices
where the parent driver manages data access.

> 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 skipping the memory mapping here cause a NULL pointer dereference in
exported APIs?  When pmt_telem_read32() is called on a callback-enabled
endpoint, it unconditionally dereferences ep->base without checking:

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

Since this patch leaves entry->base (and subsequently ep->base) as NULL,
will this result in a kernel panic when consumers use pmt_telem_read32()?

[Severity: Medium]
Similarly, does skipping the map break the telemetry_region API contract?
In pmt_copy_region(), the region address is populated using ep->base:

drivers/platform/x86/intel/pmt/telemetry.c:pmt_copy_region() {
    ...
    region->addr = entry->ep->base;
    region->size = entry->size;
    ...
}

This sets region->addr to NULL for callback-enabled devices. Existing
consumers expect a valid address, and the KUnit test for PMT regions
explicitly fails on this:

drivers/platform/x86/intel/pmt/discovery-kunit.c:validate_pmt_regions() {
    ...
    KUNIT_ASSERT_NOT_ERR_OR_NULL(test, (__force const void *)region->addr);
    ...
}

>  
>  	sysfs_bin_attr_init(&entry->pmt_bin_attr);
>  	entry->pmt_bin_attr.attr.name = ns->name;
>  	entry->pmt_bin_attr.attr.mode = 0440;
>  	entry->pmt_bin_attr.mmap = intel_pmt_mmap;

[Severity: High]
This is a pre-existing issue, but does assigning this mmap handler
unconditionally expose telemetry memory and bypass the callbacks?

If a userspace process calls mmap on the PMT sysfs file, the
intel_pmt_mmap handler maps the raw physical memory directly into
userspace using io_remap_pfn_range().  For endpoints that provide a
read_telem callback, they rely on software mediation to safely access
data (like waking the device, acquiring locks, etc.).

Could userspace completely circumvent these protections and potentially
cause hardware lockups or incorrect data reads by mapping the physical
memory directly?

>  	entry->pmt_bin_attr.read = intel_pmt_read;
>  	entry->pmt_bin_attr.size = entry->size;
>

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