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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Sep 11, 2026 at 08:26:14PM +0000, sashiko-bot@kernel.org wrote:
> 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.

I believe we need to address this...

> 
> 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