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 for Android: free password hash cracker in your pocket

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

Message-ID: <Z9An8TJ37Ok8BRNQ@intel.com>
Date: Tue, 11 Mar 2025 20:09:21 +0800
From: Chao Gao <chao.gao@...el.com>
To: "Chang S. Bae" <chang.seok.bae@...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 Mon, Mar 10, 2025 at 10:33:20AM -0700, Chang S. Bae wrote:
>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.

Hi Chang,

All of the above makes sense to me. Thank you for your review and suggestions.

I will update the changelog to reference the recent change in tip-tree and post
it separately.

One thing I'm not entirely clear on is "the fix is only partial". I assume I
need to update gfpu->perm to reference fpu_kernel_cfg to complement the fix.
Is that correct?