Re: [Patch v9 12/24] perf/x86: Enable XMM register sampling for REGS_USER case

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 7/6/2026 10:35 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] For PEBS events, a delayed PMI NMI hitting in kernel mode causes REGS_INTR to overwrite REGS_USER's XMM registers with the current hardware FPU state, leaking kernel FPU data.
> - [High] PERF_SAMPLE_REGS_USER incorrectly samples Guest XMM registers paired with Host GPRs when a PMI hits during KVM guest execution, leaking guest state to the host.
> --
>
> commit 74494b0320a832fddee80ea2906067a8bc2b32e0
> Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> perf/x86: Enable XMM register sampling for REGS_USER case
>
> This commit introduces support for XMM register sampling in the REGS_USER
> case. It adds a per-CPU x86_user_regs to store REGS_USER-specific XMM
> registers and a helper x86_pmu_update_user_xregs() to retrieve them either
> from cached task FPU state or hardware registers.
>
>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
>> index 3e52610510cdc..0d42c51761f9d 100644
>> --- a/arch/x86/events/core.c
>> +++ b/arch/x86/events/core.c
> [ ... ]
>
>> @@ -1812,33 +1806,165 @@ 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;
>> +	unsigned int guest_state;
>> +	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]
> Can this incorrectly sample Guest XMM registers paired with Host GPRs when
> a PMI hits during KVM guest execution?
>
> Since TIF_NEED_FPU_LOAD is cleared when KVM actively runs a guest, this
> condition evaluates to false, bypassing the fpu->__task_fpstate memory
> load.
>
> If the guest is running in user mode, perf_guest_state() & PERF_GUEST_USER
> will be true, causing this function to return user_mask. Consequently,
> xsaves_nmi() will sample the live hardware FPU containing the guest's
> registers instead of QEMU's.

Hmm, it's true. Since currently guest fpu are lazily swapped, as long as
PMI hits the guest, the FPU hardware would always contain the guest FPU
states. It looks we need to do below change and ensure the FPU state of
user space can be always retrieved when PMI hits guest.

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 323be08778d6..b78d4f261b76 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1978,7 +1978,8 @@ static inline u64 x86_pmu_update_user_xregs(struct
perf_sample_data *data,
        if (user_mode(regs))
                user_mask &= ~ignore_mask;

-       if (user_mask && test_thread_flag(TIF_NEED_FPU_LOAD)) {
+       if (user_mask &&
+           (test_thread_flag(TIF_NEED_FPU_LOAD) || perf_guest_state())) {
                perf_regs = container_of(data->regs_user.regs,
                                 struct x86_perf_regs, regs);
                fpu = x86_task_fpu(current);
@@ -1995,15 +1996,6 @@ static inline u64 x86_pmu_update_user_xregs(struct
perf_sample_data *data,
                return 0;
        }

-       guest_state = perf_guest_state();
-       /*
-        * Skip SIMD register sampling if a PMI hits while guest kernel
-        * state is still active.
-        */
-       if (user_mask && (guest_state & PERF_GUEST_ACTIVE) &&
-           !(guest_state & PERF_GUEST_USER))
-               return 0;
-


>
>> +		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);
> [ ... ]
>
>>  static void x86_pmu_sample_xregs(struct perf_event *event,
>>  				 struct perf_sample_data *data,
>> +				 struct pt_regs *regs,
>>  				 u64 ignore_mask)
>>  {
> [ ... ]
>
>> +	if (user_mask) {
>> +		perf_regs = container_of(data->regs_user.regs,
>> +					 struct x86_perf_regs, regs);
>> +		update_perf_regs(perf_regs, xsave, user_mask);
>> +	}
>> +
>>  	if (intr_mask) {
>>  		perf_regs = container_of(data->regs_intr.regs,
>>  					 struct x86_perf_regs, regs);
>> -		xsave->header.xfeatures = 0;
>> -		xsaves_nmi(xsave, mask);
>>  		update_perf_regs(perf_regs, xsave, intr_mask);
> [Severity: High]
> For PEBS events, if a delayed PMI NMI hits in kernel mode, can REGS_INTR
> overwrite REGS_USER's XMM registers and leak kernel FPU data?
>
> If a PEBS event requests both PERF_SAMPLE_REGS_USER and
> PERF_SAMPLE_REGS_INTR, and a PEBS record is generated in user space but
> the PMI is delivered while the CPU is in kernel mode (after the kernel has
> set TIF_NEED_FPU_LOAD):
>
> x86_pmu_update_regs_user() sets:
>     data->regs_user.regs = regs;
>     
> x86_pmu_update_regs_intr() sets:
>     data->regs_intr.regs = regs;
>     
> Both now share the same x86_perf_regs container. In x86_pmu_sample_xregs(),
> the container is first updated with the user's FPU state, but then it is
> unconditionally overwritten with the kernel's live FPU state from
> xsaves_nmi() here because intr_mask is non-zero.

Yeah, the risk is always possible. To mitigate the risk, we may need to use
an independent x86_perf_regs at any time, like this,

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 323be08778d6..9f8f436e6518 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1912,10 +1912,7 @@ static void x86_pmu_update_regs_user(struct
perf_event *event,
        struct perf_event_attr *attr = &event->attr;
        struct x86_perf_regs *perf_regs;

-       if (user_mode(regs)) {
-               data->regs_user.abi = perf_reg_abi(current);
-               data->regs_user.regs = regs;
-       } else if (is_user_task(current)) {
+       if (user_mode(regs)) || is_user_task(current)) {
                /*
                 * It cannot guarantee that the kernel will never
                 * touch the registers outside of the pt_regs,


>
>>  	}
>>  }