linux-kernel - Re: [PATCH v3 04/10] x86/fpu/xstate: Correct guest fpstate size calculation

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: <b624a831-0c91-4e89-8183-a9a1ea569e6c@intel.com>
Date: Mon, 10 Mar 2025 10:33:20 -0700
From: "Chang S. Bae" <chang.seok.bae@...el.com>
To: Chao Gao <chao.gao@...el.com>
CC: <tglx@...utronix.de>, <dave.hansen@...el.com>, <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 04/10] x86/fpu/xstate: Correct guest fpstate size
 calculation

On 3/10/2025 12:06 AM, Chao Gao wrote:
> 
> Should patch 2 be posted separately?

gfpu->perm has been somewhat overlooked, as __xstate_request_perm() does 
not update this field. However, I see that as a separate issue. The 
options are either to fix it so that it remains in sync with 
fpu->guest_perm consistently or to remove it entirely, as you proposed, 
if it has no actual use.

There hasn’t been any relevant change that would justify a quick 
follow-up like the other case. So, I'd assume it as part of this series.

But yes, I think gfpu->perm is also going to be 
fpu_kernel_cfg.default_features at the moment.

> Regarding the changelog, I am uncertain what's quite different in the context.
> It seems both you and I are talking about the inconsistency between
> gfpu->xfeatures and fpstate->xfeatures. Did I miss something obvious?

I saw a distinction between inconsistencies within a function and 
inconsistencies across functions.

Stepping back a bit, the approach for defining the VCPU xfeature set was 
originally intended to include only user features, but it now appears 
somewhat inconsistent:

(a) In fpu_alloc_guest_fpstate(), fpu_user_cfg is used.
(b) However, __fpstate_reset() references fpu_kernel_cfg to set storage
     attributes.
(c) Additionally, fpu->guest_perm takes fpu_kernel_cfg, which affects
     fpstate_realloc().

To maintain a consistent VCPU xfeature set, (b) and (c) should be corrected.

Alternatively, the VCPU xfeature set could be reconsidered to align with 
how other tasks handle it. This might offer better maintainability 
across functions. In that case, another option would be simply updating 
fpu_alloc_guest_fpstate().

The recent tip-tree change seems somewhat incomplete — perhaps in 
hindsight. If following up on this, the changelog should specifically 
address inconsistencies within a function. I saw this as a way to 
solidify an upcoming change, where addressing it sooner rather than 
later would be beneficial.

In patch 3, you've pointed out the inconsistency between (a) and (b), 
which is a valid point. However, the fix is only partial and does not 
fully address the issue either. Moreover, the patch does not reference 
the recent tip-tree change as it didn't have any context at that time.

Thanks,
Chang