Re: [PATCH V3 7/7] perf/x86/intel/uncore: Implement global init callback for GNR uncore

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Hardware trust boundary violation: A spoofed PCIe device can supply an arbitrary `global.ctl` value, leading to arbitrary MSR writes.
- [Medium] The newly added `uncore_msr_global_init()` uses the unchecked `wrmsrq_on_cpu()` function to write to an MSR address obtained from the hardware discovery table.
--

commit b0a74f9b5d22ecfcf334d277878f153181fb3107
Author: Zide Chen <zide.chen@intel.com>

perf/x86/intel/uncore: Implement global init callback for GNR uncore

On Sierra Forest and Clearwater Forest, the FRZ_ALL bit in the global
control register defaults to 0 at boot, but UBOX PMON units do not
work until the global control register is explicitly written with 0
to trigger hardware initialization properly.

> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
[ ... ]
> @@ -1731,6 +1731,16 @@ static int uncore_mmio_global_init(u64 ctl)
>  	return 0;
>  }
>  
> +static int uncore_msr_global_init(int die, u64 msr)
> +{
> +	int cpu = uncore_die_to_cpu(die);
> +
> +	if (cpu == -1)
> +		return -ENODEV;
> +
> +	return wrmsrq_on_cpu(cpu, msr, 0);

[Severity: Medium]
Since the msr parameter is derived from global.ctl which is read from
the hardware discovery table, could an incorrect value from firmware trigger
an unchecked MSR access warning?  Would it be better to use
wrmsrq_safe_on_cpu() here to prevent scary traces if the hardware table
is invalid?

> +}
> +
>  static const struct uncore_plat_init nhm_uncore_init __initconst = {
[ ... ]
> diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
> --- a/arch/x86/events/intel/uncore_discovery.c
> +++ b/arch/x86/events/intel/uncore_discovery.c
[ ... ]
> @@ -287,7 +287,7 @@ static int __parse_discovery_table(struct uncore_discovery_domain *domain,
>  	if (!io_addr)
>  		return -ENOMEM;
>  
> -	if (domain->global_init && domain->global_init(global.ctl)) {
> +	if (domain->global_init && domain->global_init(die, global.ctl)) {

[Severity: Critical]
Does this allow an external device to write 0 to arbitrary MSRs?

The uncore discovery code scans the PCI bus for devices matching the Intel
Vendor ID and discovery Device ID without restricting the search to internal
CPU buses.  If a malicious external PCIe device (e.g., via Thunderbolt)
spoofs these IDs, it can provide a fake discovery table.

Could this attacker-controlled global.ctl value then be passed to
uncore_msr_global_init() and used as the msr index, allowing an attacker
to overwrite security-sensitive MSRs like MSR_LSTAR or MSR_GS_BASE with 0?

>  		ret = -ENODEV;
>  		goto out;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602144908.263680-1-zide.chen@intel.com?part=7