Re: [PATCH v4 06/18] 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:
- [High] NULL pointer dereference in `pmt_telem_read32()` when endpoints use telemetry callbacks.
- [High] The sysfs `.mmap` handler unconditionally maps physical telemetry memory to userspace, bypassing the newly added parent driver read callback.
--

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

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

This commit updates the PMT class driver to skip creating a memory remap
when a read_telem callback is provided. It intends to let the parent
driver handle data area access when a callback is present.

> diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
> index af7c951b569a2..350129121ff13 100644
> --- a/drivers/platform/x86/intel/pmt/class.c
> +++ b/drivers/platform/x86/intel/pmt/class.c

[ ... ]

> @@ -403,14 +402,18 @@ 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;
> +	if (!is_cb_read_telem(entry->cb)) {
> +		struct resource res = {0};
>  
> -	entry->base = devm_ioremap_resource(dev, &res);
> -	if (IS_ERR(entry->base)) {
> -		ret = PTR_ERR(entry->base);
> -		goto fail_ioremap;
> +		res.start = entry->base_addr;
> +		res.end = res.start + entry->size - 1;
> +		res.flags = IORESOURCE_MEM;
> +
> +		entry->base = devm_ioremap_resource(dev, &res);
> +		if (IS_ERR(entry->base)) {
> +			ret = PTR_ERR(entry->base);
> +			goto fail_ioremap;
> +		}
>  	}

[Severity: High]
Does this leave entry->base uninitialized (NULL) when a telemetry callback
is present?

If a callback is provided, we skip mapping the device memory. However, the
exported pmt_telem_read32() API unconditionally dereferences ep->base without
checking for the callback:

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

Unlike pmt_telem_read(), which safely delegates to pmt_telem_read_mmio(),
any driver invoking pmt_telem_read32() on a callback-enabled endpoint might
trigger a NULL pointer dereference.

>  
>  	sysfs_bin_attr_init(&entry->pmt_bin_attr);

[Severity: High]
Since the intel_pmt_mmap handler is unconditionally assigned to the sysfs
binary attribute just below this, can userspace map the physical telemetry
memory directly, bypassing the parent driver's read callback?

When userspace mmaps the sysfs file, intel_pmt_mmap() handles it:

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;
        ...
}

This directly maps the physical memory to userspace using
io_remap_pfn_range(), which seems to defeat the callback-based access
control and hardware state management intended by skipping the ioremap.

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