Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d7de8134-b3ff-35ef-0b01-187ad2ccb771@intel.com>
Date:   Tue, 11 Oct 2022 15:47:26 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH] x86/fpu: Remove dynamic features from xcomp_bv for
 init_fpstate
Content-Language: en-CA
To:     Dave Hansen <dave.hansen@linux.intel.com>,
        <linux-kernel@vger.kernel.org>
CC:     <x86@kernel.org>, Yuan Yao <yuan.yao@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>
References: <20221011222425.866137-1-dave.hansen@linux.intel.com>
From:   "Chang S. Bae" <chang.seok.bae@intel.com>
In-Reply-To: <20221011222425.866137-1-dave.hansen@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UFBPRW5sY1c0eFYxQWR4VnVXV3ZKT092eE92SStIVmdtbWl4VzVVdldSNUJm?=
 =?utf-8?B?LzMrMmR1dTNwQkQzY2N6UTNNZW42cUJDbXRmUnhnYlBFRzMvWUVHZ25MMzcw?=
 =?utf-8?B?YlJjazZSWEsxWWZBUlUrVkJhc0xGWno0ZDVGOHQyenEvd2Z1dW1JZEN3MGJM?=
 =?utf-8?B?dmRqMUloYktNeXE2UU9XVGxOQUsxQU5QaTJydTVpd2VueVBsckJMaitObSt6?=
 =?utf-8?B?aGFvK09FMWRQUHg0ODRmRFdCdHdQTXFTeEt0TWFLZXFzQ1RIcm5yS3RvMHQ4?=
 =?utf-8?B?TjVhMnMxTmFVRC9TQldMQmhGSXRvSGZTYnRicVYwZDR0bTdhdFZkWHBiQlRl?=
 =?utf-8?B?eS8xZnFkZ3lkQUhaTEorMHZianltOU0rZnFOMkc1RDhCM29rSjk4UlBUdDFK?=
 =?utf-8?B?RENFRkwwcGN2UlQvNWt0YkVzUm1YNVpRa2E4bCs3dll6b0k0SGU2MlpVV3ky?=
 =?utf-8?B?NjRnZEVQUHJVNjlyYW0rZHZEdFNSd0VtN2ZoRGgrQ2R1amE4NHk3bmJpK0Rs?=
 =?utf-8?B?VzNTeUQ4dlFJaDBNdjloeDUrR09sTm03dVNHaFVjWWFhdnlXT29jemN4RmtY?=
 =?utf-8?B?Tm9aRWpzeDhITUlEUTQwQTJyRmZ6RlZEekFDRWQyUkFjVzNBc0c5RWd3TjFi?=
 =?utf-8?B?V3dLUFNNd0JlUkQ3ak41YjRlcHJONStjL05yQUxnbUtzNGhWTXR6VVJmRytB?=
 =?utf-8?B?cHF2TTMwU0pUeEtmbktsb0lYZXJJcTh0cE1NdjZUaWFRNERGL0hPd254WWl4?=
 =?utf-8?B?eTVTbVFTb2plK0FQQjV3eGJlZnovdUJXYnhuQmtaeUFVdGUxTlgwYll0anBE?=
 =?utf-8?B?ZnpoUVpSYmNvWVhBQ0xxZ2Fnd2dKZmtDdDM2WElUanczSU1IRFZzQ0ZCeXJk?=
 =?utf-8?B?Z3FjSVdHKzJlYVplenZCejY3UzV2RHA4aGwzdUtWbVFidGRscm9QRjhKUU1V?=
 =?utf-8?B?VFkvek1IZFpYeW9PeUFQbUtqd2lwWHJpYXl4Qzl1OUtVNDdGMzQ2Q1NUdHpS?=
 =?utf-8?B?Ny9UdWhKRFM2bVN4eTI1Ny9jQXUzdzVLUEJHSlpiNFJHVXJMcEJyaEJuay94?=
 =?utf-8?B?QXBuM201bkVocVNvd09wM2xJTkxHUzZYQ1RuMVBSYWFTVlcrZUNzaTkxUXVj?=
 =?utf-8?B?dVcyUU5rcVR1UEc3bTlyUFFWRUtNdXVUc3lmeThFMVlYQUVlVVpzVzZ4c21L?=
 =?utf-8?B?d1BhRlo2T1JxNEJjVnkva1AvM3d6WktiYzg0eWpuaUJMNENkTlVVK1l2d3kw?=
 =?utf-8?B?VUtHbmdIbTh6ODF4QldrM0U1RmRrV0JGOTVJZ2RuUTNMN0tIMFVYSXhRQ2Ux?=
 =?utf-8?B?THlWanN4eVR2MzM0TXZBVWxhZnBFeThobkh6aUgvUk1oQk5zNnBlbmZuMWU5?=
 =?utf-8?B?NGpFOTl0K2hDWjFVWWQyVUFFSGkzK1BSQXNiRHVFOWdUK2U5ZEFCd0NlMklV?=
 =?utf-8?B?a29QY1lRcjgzcXpPNFZIUUtZM05BS2l3dDJ3K1hqRFQvSDJTWmhENVJrd3lr?=
 =?utf-8?B?Q25NODlOMUhUZTMrM0xJaEFnQkVleGE3SVR0aDI1bW5nQkRmbWtxbkN5V3lD?=
 =?utf-8?B?dlU3TzNGZTJLUm9UU0VpY3J0Y3ZsSjVER0RnME8wZzEwZThZSWVIRlZha0xR?=
 =?utf-8?B?S3dQMXBjYm56TmNwelhwOVY5Yk45RjVRT0VaM0paSHV2eGJPV2NCUWFTVjU1?=
 =?utf-8?B?bFdQMFZZcVJjbk9oRkRHdXVsdHBqcEhIYVV6WFc4cW52djU0OWpsZFBXWlhP?=
 =?utf-8?B?ajRWeEtLdmV3NjBGRkJkdjlPZUpqUC91bG83QU5ObXFEeU9ITTZ5NFBkUjlS?=
 =?utf-8?B?R3BNbkVwQ3lyOTd2M1IzNTVmdzVQanI0VHBlM1ozN2s1bHpvVjBJVFN6dXNU?=
 =?utf-8?B?ZUIzMU5mZFNGeWJjVThIRVVRL3UzK2ZFUnc2cXBxNXEyMXY1b1ZEeTU0VVpz?=
 =?utf-8?B?bU52aEhzSHk1QTZqK0F4VlZXSDJPaUFRUnNGNUtLOUpXZjFwVXlrMGQzaU9W?=
 =?utf-8?B?eHFKNWFLNDVhZUNNek1kTVFXMWdNbzA4OWJEUUdaV0hjaDlmSlcvKy9pOWlw?=
 =?utf-8?B?NDNpdTZieGhJdktseTJoTkxFTU1ObDA2Qzg5TXhqUXUvQUxvOUtvdGVjQklN?=
 =?utf-8?B?a0hNWEZ5a3NLWDNXU1Fwb2NBbVhJUE5oUXJjU3NEbGtacldKQ1hvMHBSRnJX?=
 =?utf-8?B?c2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3a01d90a-e6c0-48de-7146-08daabda92b1
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4855.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Oct 2022 22:47:28.0231
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: M8t51JPa+ntmeRfs8sgZmAshNmgDb/iUC3EuNfHaALxhYrhpKb4NcPmVN7LM1wPpSIWKavWUrierO+xTOndqppGd+aD6wjdOW4DPbOatVF4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6383
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 10/11/2022 3:24 PM, Dave Hansen wrote:
> From: Yuan Yao <yuan.yao@intel.com>
> 
> This was found a couple of months ago in a big old AMX
> backport.  But, it looks to be a problem in mainline too.
> Please let me know if this looks OK.  I'd also especially
> appreciate some testing from folks that have AMX hardware
> handy.
> 
> Builds and survives a quick boot test on non-AMX hardware.
> 
> --
> 
> == Background ==
> 
> 'init_fpstate' is a sort of template for all of the fpstates
> that come after it.  It is copied when new processes are
> execve()'d or XRSTOR'd to get fpregs into a known state.
> 
> That means that it represents the *starting* state for a
> process's fpstate which includes only the 'default' features.
> Dynamic features can, of course, be acquired later, but
> processes start with only default_features.
> 
> During boot the kernel decides whether all fpstates will be
> kept in the compacted or uncompacted format.  This choice is
> communicated to the hardware via the XCOMP_BV field in all
> XSAVE buffers, including 'init_fpstate'.
> 
> == Problem ==
> 
> But, the existing XCOMP_BV calculation is incorrect.  It uses
> the set of 'max_features', not the default features.
> 
> As a result, when XRSTOR operates on buffers derived from
> 'init_fpstate', it may attempt to "tickle" dynamic features which
> are at offsets for which there is no space allocated in the
> fpstate.
> 
> == Scope ==
> 
> This normally results in a relatively harmless out-of-bounds
> memory read.  It's harmless because it never gets consumed.  But,
> if the fpstate is next to some unmapped memory, this "tickle" can
> cause a page fault and an oops.
> 
> This only causes issues on systems when dynamic features are
> available and when an XSAVE buffer is next to uninitialized
> memory.  In other words, it only affects recent Intel server
> CPUs, and in relatively few memory locations.
> 
> Those two things are why it took relatively long to catch this.
> 
> == Solution ==
> 
> Use 'default_features' to establish the init_fpstate
> xcomp_bv value.  Reset individual fpstate xcomp_bv values
> when the rest of the fpstate is reset.
> 
> [ dhansen: add reset code from tglx, rewrites
> 	   commit message and comments ]
> 
> Fixes: 1c253ff2287f ("x86/fpu: Move xstate feature masks to fpu_*_cfg")
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Yuan Yao <yuan.yao@intel.com>
> Cc: stable@vger.kernel.org
> ---
>   arch/x86/kernel/fpu/core.c   | 3 +++
>   arch/x86/kernel/fpu/xstate.c | 7 ++++++-
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 3b28c5b25e12..4d64de34da12 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -526,6 +526,9 @@ static void __fpstate_reset(struct fpstate *fpstate, u64 xfd)
>   	fpstate->xfeatures	= fpu_kernel_cfg.default_features;
>   	fpstate->user_xfeatures	= fpu_user_cfg.default_features;
>   	fpstate->xfd		= xfd;
> +
> +	/* Ensure that xcomp_bv matches ->xfeatures */
> +	xstate_init_xcomp_bv(&fpstate->regs.xsave, fpstate->xfeatures);
>   }
>   
>   void fpstate_reset(struct fpu *fpu)
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c8340156bfd2..f9f45610c72f 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -360,7 +360,12 @@ static void __init setup_init_fpu_buf(void)
>   
>   	print_xstate_features();
>   
> -	xstate_init_xcomp_bv(&init_fpstate.regs.xsave, fpu_kernel_cfg.max_features);
> +	/*
> +	 * 'init_fpstate' is sized for the default feature
> +	 * set without any of the dynamic features.
> +	 */
> +	xstate_init_xcomp_bv(&init_fpstate.regs.xsave,
> +			     fpu_kernel_cfg.default_features);

Along with this, perhaps this hunk needs to be updated as well [2]:

In arch/x86/kernel/fpu/init.c,

static void __init fpu__init_init_fpstate(void)
{
	/* Bring init_fpstate size and features up to date */
	init_fpstate.size		= fpu_kernel_cfg.max_size;
	init_fpstate.xfeatures		= fpu_kernel_cfg.max_features;
}

I attempted to fix the issue in the earlier posting [1]. But, mine looks 
to be clearly missing that __fpstate_reset() part.

Thanks,
Chang

[1]: 
https://lore.kernel.org/lkml/20220824191223.1248-1-chang.seok.bae@intel.c
[2]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/init.c#n213