Re: [PATCH] platform/x86/intel: power-domains validate domain in tpmi_cpu_online()

[David Arcari]

Hi Srinivas,

On 8/26/25 4:26 PM, srinivas pandruvada wrote:
> Hi David,
>
> On Tue, 2025-08-26 at 12:43 -0400, David Arcari wrote:
> > Although tpmi_get_power_domain_mask() calls tpmi_domain_is_valid()
> > prior to indexing tpmi_power_domain_mask[],
> Because this an API call so that caller parameter needs to be
> sanitized.
>
> >   tpmi_cpu_online() does
> > not.
> This is hotplug callback, which should have correct topology
> information.
>
> >   In the case where a VM creates non-contiguous package ids the
> > result can be memory corruption. This can be prevented by adding
> > the same validation in tpmi_cpu_online().
>
> This driver is getting loaded means MSR 0x54 is virtualised otherwise
> this driver will not load.

I don't have direct access to the system, but this appears to be the 
case.  The driver is reading MSR 0x54:

drivers/platform/x86/intel/tpmi_power_domains.c:#define 
MSR_PM_LOGICAL_ID       0x54
drivers/platform/x86/intel/tpmi_power_domains.c:        ret = 
rdmsrl_safe(MSR_PM_LOGICAL_ID, &data);
drivers/platform/x86/intel/tpmi_power_domains.c:        ret = 
rdmsrl_safe(MSR_PM_LOGICAL_ID, &data);


> Not sure this is an upstream kernel or not.

This was not an upstream kernel, but I don't see anything in the 
upstream driver that would have prevented the access that is occurring.

> Some comments below.
>
> > Fixes: 17ca2780458c ("platform/x86/intel: TPMI domain id and CPU
> > mapping")
> Andy already pointed about new line here.
>
> > Cc: Hans de Goede <hansg@xxxxxxxxxx>
> > Cc: "Ilpo Järvinen" <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > Cc: David Arcari <darcari@xxxxxxxxxx>
> > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Cc: Tero Kristo <tero.kristo@xxxxxxxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Signed-off-by: David Arcari <darcari@xxxxxxxxxx>
> > ---
> >   drivers/platform/x86/intel/tpmi_power_domains.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/platform/x86/intel/tpmi_power_domains.c
> > b/drivers/platform/x86/intel/tpmi_power_domains.c
> > index 9d8247bb9cfa..ae5b58679e29 100644
> > --- a/drivers/platform/x86/intel/tpmi_power_domains.c
> > +++ b/drivers/platform/x86/intel/tpmi_power_domains.c
> > @@ -194,6 +194,9 @@ static int tpmi_cpu_online(unsigned int cpu)
> >   	if (ret)
> >   		return 0;
> >   
> Need some more information.
>
> The only case this check is required if
> topology_physical_package_id(cpu) is returning greater or equal to
> topology_max_packages(). If this true in this case, please check the
> value of info->pkg_id. If this is bad then then some other places also
> this may have issue. info->punit_domain_id is already checked for valid
> value in tpmi_get_logical_id().

That is correct.  In the case of the crash we have:

crash> p/x __max_logical_packages
$1 = 0x2

static inline unsigned int topology_max_packages(void)
{
	return __max_logical_packages;
}


$2 = {
  hnode = {
    next = 0xffff9651bbc37010,
    pprev = 0xffffffffc0b7a640 <tpmi_cpu_hash>
  },
  linux_cpu = 1,
  pkg_id = 2 '\002',
  punit_thread_id = 0 '\000',
  punit_core_id = 0 '\000',
  punit_domain_id = 0 '\000'
}

The pkg_id of 2 leads to the bad reference.

FWIW this change has been tested and resolves the issue.

Let me know if there is any other information I can provide.  I will be 
out of the office on Wednesday, so response may be delayed.

Best,
-DA

> Thanks,
> Srinivas
>
> > +	if (!tpmi_domain_is_valid(info))
> > +		return 0;
> > +
> >   	index = info->pkg_id * MAX_POWER_DOMAINS + info-
> > > punit_domain_id;
> >   
> >   	guard(mutex)(&tpmi_lock);