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

[David Arcari <darcari@redhat.com>]

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@kernel.org>
>> Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
>> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Dan Carpenter <dan.carpenter@linaro.org>
>> Cc: David Arcari <darcari@redhat.com>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Tero Kristo <tero.kristo@linux.intel.com>
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: David Arcari <darcari@redhat.com>
>> ---
>>   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);
> 
>