Re: [Patch v8 11/23] perf/x86: Enable XMM register sampling for REGS_USER case

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Calling `xsaves_nmi()` for PEBS events on CPUs lacking `X86_FEATURE_XSAVES` causes an invalid opcode exception (`#UD`), triggering warning loops and overhead.
- [High] Guest XMM registers are incorrectly sampled as host REGS_USER XMM registers when a PMU interrupt occurs during KVM guest execution.
--

commit feabaffcb00c51c951e3f0889a9ad615fe731d5e
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86: Enable XMM register sampling for REGS_USER case

This patch adds support for XMM register sampling in the REGS_USER case.
To handle simultaneous sampling of XMM registers, a per-CPU x86_user_regs
is introduced to store REGS_USER-specific XMM registers. The
x86_pmu_update_user_xregs helper function is added to retrieve user-space
XMM registers.

> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -707,12 +707,12 @@ int x86_pmu_hw_config(struct perf_event *event)
>  			return -EINVAL;
>  	}
>  
> -	if (event->attr.sample_type & PERF_SAMPLE_REGS_INTR) {
> +	if (event->attr.sample_type & (PERF_SAMPLE_REGS_INTR | PERF_SAMPLE_REGS_USER)) {
>  		/*
>  		 * Besides the general purpose registers, XMM registers may
>  		 * be collected as well.
>  		 */
> -		if (event->attr.sample_regs_intr & PERF_REG_EXTENDED_MASK) {
> +		if (event_has_extended_regs(event)) {
>  			if (!(event->pmu->capabilities & PERF_PMU_CAP_EXTENDED_REGS))
>  				return -EINVAL;
>  			if (is_sampling_event(event) && !event->attr.precise_ip &&
>  			    !this_cpu_has(X86_FEATURE_XSAVES))
>  				return -EINVAL;
>  		}
>  	}
[ ... ]
> @@ -1812,33 +1803,155 @@ static void x86_pmu_update_regs_intr(struct perf_event *event,
[ ... ]
> +static inline u64 x86_pmu_update_user_xregs(struct perf_sample_data *data,
> +					    struct pt_regs *regs,
> +					    u64 mask, u64 ignore_mask)
> +{
> +	struct x86_perf_regs *perf_regs;
> +	struct xregs_state *xsave;
> +	struct fpu *fpu;
> +	struct fpstate *fps;
> +	u64 user_mask = mask;
> +
> +	if (data->regs_user.abi == PERF_SAMPLE_REGS_ABI_NONE)
> +		return 0;
> +
> +	/*
> +	 * If PEBS hits kernel space, need to re-sample extended
> +	 * registers for user space.
> +	 */
> +	if (user_mode(regs))
> +		user_mask &= ~ignore_mask;
> +
> +	if (user_mask && test_thread_flag(TIF_NEED_FPU_LOAD)) {

[Severity: High]
When KVM loads a guest's FPU state, fpu_swap_kvm_fpstate() caches the host's
state in fpu->__task_fpstate and explicitly clears the TIF_NEED_FPU_LOAD flag
via fpregs_mark_activate().

If a perf NMI interrupts the guest, this check for TIF_NEED_FPU_LOAD will
evaluate to false, causing x86_pmu_update_user_xregs() to return the mask
instead of reading the cached host state from __task_fpstate.

Does this cause xsaves_nmi() to sample the hardware registers directly, which
currently hold the KVM guest's XMM registers, and expose them to the host's
user-space perf record, leading to cross-VM data leaks?

> +		perf_regs = container_of(data->regs_user.regs,
> +				 struct x86_perf_regs, regs);
> +		fpu = x86_task_fpu(current);
> +		/*
> +		 * If __task_fpstate is set, it holds the right pointer,
> +		 * otherwise fpstate will.
> +		 */
> +		fps = READ_ONCE(fpu->__task_fpstate);
> +		if (!fps)
> +			fps = fpu->fpstate;
> +		xsave = &fps->regs.xsave;
> +
> +		update_perf_regs(perf_regs, xsave, user_mask);
> +		return 0;
> +	}
> +
> +	return user_mask;
> +}
[ ... ]
>  static void x86_pmu_sample_xregs(struct perf_event *event,
>  				 struct perf_sample_data *data,
> +				 struct pt_regs *regs,
>  				 u64 ignore_mask)
>  {
>  	struct xregs_state *xsave = get_ext_regs_buf(smp_processor_id());
>  	u64 sample_type = event->attr.sample_type;
>  	struct x86_perf_regs *perf_regs;
> +	u64 user_mask = 0;
>  	u64 intr_mask = 0;
>  	u64 mask = 0;
>  
>  	if (WARN_ON_ONCE(!xsave) || !in_nmi())
>  		return;
>  
> -	if ((sample_type & PERF_SAMPLE_REGS_INTR) &&
> -	    (event->attr.sample_regs_intr & PERF_REG_EXTENDED_MASK))
> +	if (event_has_extended_regs(event))
>  		mask |= XFEATURE_MASK_SSE;
>  
>  	mask &= x86_pmu.ext_regs_mask;
> +	if (sample_type & PERF_SAMPLE_REGS_USER) {
> +		user_mask = x86_pmu_update_user_xregs(data, regs,
> +						      mask, ignore_mask);
> +	}
>  
>  	if (sample_type & PERF_SAMPLE_REGS_INTR)
>  		intr_mask = mask & ~ignore_mask;
>  
> +	if (user_mask | intr_mask) {
> +		xsave->header.xfeatures = 0;
> +		xsaves_nmi(xsave, user_mask | intr_mask);
> +	}

[Severity: High]
For PEBS events with PERF_SAMPLE_REGS_USER, the X86_FEATURE_XSAVES check in
x86_pmu_hw_config() is bypassed due to event->attr.precise_ip being set.

If this event triggers in kernel space on a CPU lacking XSAVES (for example,
booted with noxsaves or masked in a VM), user_mode(regs) is false, so
user_mask retains the extended bits from x86_pmu_update_user_xregs().

Since xsaves_nmi() unconditionally emits the xsaves instruction, will this
trigger an Invalid Opcode exception on CPUs without XSAVES support and add
overhead to every PEBS interrupt in kernel space?

> +
> +	if (user_mask) {
> +		perf_regs = container_of(data->regs_user.regs,
> +					 struct x86_perf_regs, regs);
> +		update_perf_regs(perf_regs, xsave, user_mask);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529075645.580362-1-dapeng1.mi@linux.intel.com?part=11