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

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

On Mon, Sep 14, 2026 at 05:16:56PM -0400, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> >Sent: Monday, September 14, 2026 12:06 PM
> >To: sashiko-reviews@lists.linux.dev
> >Cc: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel-xe@lists.freedesktop.org
> >Subject: Re: [PATCH v8 05/20] platform/x86/intel/pmt: Do not remap when
> >using callbacks
> >
> >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...
> 
> Ok.  I did address the mmap issue in another patch set.  Can I do the same here, or do I need to
> fix this as part of this patch set?

yeap, I believe it would be good to have in this patch series before this patch.

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