linux-kernel - RE: [PATCH] x86/fpu: Remove dynamic features from xcomp_bv for init

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: <BYAPR11MB3717D733EC448197D0321FF695249@BYAPR11MB3717.namprd11.prod.outlook.com>
Date:   Fri, 14 Oct 2022 04:10:43 +0000
From:   "Yao, Yuan" <yuan.yao@...el.com>
To:     "Bae, Chang Seok" <chang.seok.bae@...el.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "x86@...nel.org" <x86@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: RE: [PATCH] x86/fpu: Remove dynamic features from xcomp_bv for
 init_fpstate

>-----Original Message-----
>From: Bae, Chang Seok <chang.seok.bae@...el.com>
>Sent: Friday, October 14, 2022 11:54
>To: Hansen, Dave <dave.hansen@...el.com>; Yao, Yuan <yuan.yao@...el.com>; Dave Hansen <dave.hansen@...ux.intel.com>;
>linux-kernel@...r.kernel.org
>Cc: x86@...nel.org; Thomas Gleixner <tglx@...utronix.de>
>Subject: Re: [PATCH] x86/fpu: Remove dynamic features from xcomp_bv for init_fpstate
>
>On 10/13/2022 10:44 AM, Dave Hansen wrote:
>>
>> A better comment for that would be:
>>
>> 	 * Some user_xfeatures may not be present in the fpstate.
>> 	 * Remove those from 'mask' to zero those features in the
>> 	 * user buffer instead of retrieving them from fpstate.
>
>Yes, indeed!
>
>Also this xstate copy routine looks to need some updates.
>
>If an xfeature is present in fpstate, and in init state, the value is
>retrieved from init_fpstate via copy_feature(). But, it has no space for
>dynamic states. Also, for extended states, the init state is known to be
>zero.
>
>Then, perhaps, init_fpstate is better not to be accessed in the
>for_each_extended_xfeature loop; instead of using copy_feature(), the
>feature can be zeroed like this:
>
>diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>index 13b83b11b3d8..0fdfd03938b6 100644
>--- a/arch/x86/kernel/fpu/xstate.c
>+++ b/arch/x86/kernel/fpu/xstate.c
>@@ -1150,11 +1150,11 @@ void __copy_xstate_to_uabi_buf(struct membuf to,
>struct fpstate *fpstate,
>                          */
>                         pkru.pkru = pkru_val;
>                         membuf_write(&to, &pkru, sizeof(pkru));
>-               } else {
>-                       copy_feature(header.xfeatures & BIT_ULL(i), &to,
>-                                    __raw_xsave_addr(xsave, i),
>-                                    __raw_xsave_addr(xinit, i),
>+               } else if (header.xfeatures & BIT_ULL(i)) {
>+                       membuf_write(&to, __raw_xsave_addr(xsave, i),
>                                      xstate_sizes[i]);
>+               } else {
>+                       membuf_zero(&to, xstate_sizes[i]);

Ah, I didn’t aware your reply before sent mine :- ) 

Does init_fpstate saves the "init" state for all non-dynamic components in its buffer ? if no than 
this change is better, else read from init_fpstate for allocated buffer and only zero the not exist buffer is preferred to me.
The head.xfeatures is 0 before the guest fpu is used to xsaves at least once.

>                 }
>
>Thanks,
>Chang