linux-kernel - Re: [PATCH v3 03/10] x86/fpu/xstate: Correct xfeatures cache in guest pseudo fpu container

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z8uvG9d59kevJcXR@intel.com>
Date: Sat, 8 Mar 2025 10:44:43 +0800
From: Chao Gao <chao.gao@...el.com>
To: Dave Hansen <dave.hansen@...el.com>
CC: <tglx@...utronix.de>, <x86@...nel.org>, <seanjc@...gle.com>,
	<pbonzini@...hat.com>, <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
	<peterz@...radead.org>, <rick.p.edgecombe@...el.com>,
	<weijiang.yang@...el.com>, <john.allen@....com>, <bp@...en8.de>
Subject: Re: [PATCH v3 03/10] x86/fpu/xstate: Correct xfeatures cache in
 guest pseudo fpu container

On Fri, Mar 07, 2025 at 09:48:25AM -0800, Dave Hansen wrote:
>On 3/7/25 08:41, Chao Gao wrote:
>> The xfeatures field in struct fpu_guest is designed to track the enabled
>> xfeatures for guest FPUs. However, during allocation in
>> fpu_alloc_guest_fpstate(), gfpu->xfeatures is initialized to
>> fpu_user_cfg.default_features, while the corresponding
>> fpstate->xfeatures is set to fpu_kernel_cfg.default_features
>> 
>> Correct the mismatch to avoid confusion.
>> 
>> Note this mismatch does not cause any functional issues. The
>> gfpu->xfeatures is checked in fpu_enable_guest_xfd_features() to
>> verify if XFD features are already enabled:
>> 
>> 	xfeatures &= ~guest_fpu->xfeatures;
>> 	if (!xfeatures)
>> 		return 0;
>> 
>> It gets updated in fpstate_realloc() after enabling some XFD features:
>> 
>> 	guest_fpu->xfeatures |= xfeatures;
>> 
>> So, backport is not needed.
>
>I don't have any great suggestions for improving this, but I just don't
>seem to find this changelog compelling. I can't put my finger on it, though.
>
>I think I'd find it more convincing if you argued what the *CORRECT*
>value is and why rather than just arguing for consistency with a random
>value. I also don't get the pivot over the XFD for explaining why it is

fpstate->xfeatures isn't a random value. It is the RFBM, right? see os_xsave().

The xfeatures in the guest FPU pesudo container (gfpu->xfeatures) is to track
enabled xfeatures of the guest FPU. I think "enabled" refers to RFBM because
only enabled features need save/restore. so gfpu->xfeatures should be
consistent with fpstate->xfeatures.

They become misaligned during allocation. Specifically, gfpu->xfeatures does
not track any supervisor features. Excluding all _supervisor_ features is
harmless, as the value is solely used to check if XFD features, which are all
_user_ features, are already enabled in fpu_enable_guest_xfd_features(). It
just causes confusion.

>harmless. XFD isn't even used in most cases, so I'd find a justification
>separate from XFD more compelling.
>

To me, there is a discrepancy between the field's name and the value it holds.
We have two options to fix it:

1. rename @xfeatures in struct fpu_guest to @user_xfeatures and update the
   comment above to state the field only tracks enabled _user_ features.

2. ensure @xfeatures in struct fpu_guest matches fpstate->xfeatures

this patch implements the option #2.