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: <9b198ed3-4b2d-c857-710b-3f7115bbcf74@intel.com>
Date:   Thu, 13 Oct 2022 09:23:20 -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:     "Yao, Yuan" <yuan.yao@intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "x86@kernel.org" <x86@kernel.org>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>
References: <20221011222425.866137-1-dave.hansen@linux.intel.com>
 <BYAPR11MB3717EDEF2351C958F2C86EED95259@BYAPR11MB3717.namprd11.prod.outlook.com>
From:   "Chang S. Bae" <chang.seok.bae@intel.com>
In-Reply-To: <BYAPR11MB3717EDEF2351C958F2C86EED95259@BYAPR11MB3717.namprd11.prod.outlook.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?T0JIQlBQMDNTdFR2NWJ1ZXBVUG9UdjR0ZFM2UGkvZFRYQ3NxcUhYQ3BDUUIw?=
 =?utf-8?B?UEkxc0VmdHphMzFPbDU5dGlEUVVWQjRtc0oyK0lFWWhrUUJmR01pdkRmM1JT?=
 =?utf-8?B?Njhsczl1ZFVncjNlcnRWcFdjdldpd29QeUo3TmoyRXI4QkFxYUtnUFEvdnRZ?=
 =?utf-8?B?ZytBcDNCVUpsZEhJZkJzNzA3SlQvNDlOSlJ5WVlQM3R5Y3E1cFprV0k5NHc1?=
 =?utf-8?B?b0V1UzI2VW5EZzhvTWdjWk9WYWlsYk01QVdBSjVGVjdoS3V3dWNvNXBnbFpZ?=
 =?utf-8?B?REZmWkdIVTNVOU5lSHd0dk1pRW5pWTlFTmx6YU5ocTlhcExjemRNeGVEcnhh?=
 =?utf-8?B?WTVoOGc4SzAwb2lQYjd3c0NGa1RNTUlPZ3RydlNGd0FIeG8veHh5QWlQZklE?=
 =?utf-8?B?NFJIV21Da2htRVNpUnZ0UGd5Zy9zMGZrS3RFeFRZSGpFZE5PbEFNdDgveHQr?=
 =?utf-8?B?M3FRbnVPZC92eFlJOFZUczVvL0Z6MHNKM0g3M05xQi9Hb1hvVjN2Q3FHZkFZ?=
 =?utf-8?B?RlRxMjY4TGhmOGpGUFJMV092Vm8xbEJUZ3BBYzlybWZtOWY0YnlBbHdYbGdo?=
 =?utf-8?B?Mm9kaksxMDhLd2hSUG9vS2hvOFlUNmpGK2x1bnVoTDZZMWRWOVN3U1JxaTEy?=
 =?utf-8?B?SkYyNHBMOHlOdXAzVGJKc0dNTmdtblZ5RVQ5MG90SkpPdFFqdkJ0bE9JbkI3?=
 =?utf-8?B?a3M4NFlweXZBeTlURGtKRm9WRysvWlhWVFpPU2lqc3F3S0U3YXlLMkx3RmZo?=
 =?utf-8?B?RGZZcURSNjVsSTVMaUJJZVNaNDNyblUwalVJQmlRQ3c1c0lPaXh1ZExhN0Z4?=
 =?utf-8?B?MkF2Umt0akxwN0RIN2R2dXA2MVI4UTZudlptNUdmcHpOaFZiWHBqVXQ5bWFZ?=
 =?utf-8?B?TGxJVmtMRVRQWGUvRkE1YXRlRkNhT0JZWVA4VThUWnJvUWJ4QlJWZXlJWU4v?=
 =?utf-8?B?TjZMKzhWcUJaZnFSalNDSWNkWGhydEVQeThSTnJXYVJEK25PTlBSZDRQSkJl?=
 =?utf-8?B?bWg1emIxMlVwVFE3SHBFQ3hnMWxha0l2ZXFJSllmanBYOXA3SnhlNkZnUDA1?=
 =?utf-8?B?ZWhLb2VjUFNzMTlGY3QwdUNTdUpHRDM2VXhTVEVFR3RLNHdYb3JLNkhtNWdj?=
 =?utf-8?B?cjk4VUo3Rlk2WnM4engyZDYraVVpaXZoandvSVJYV3lvYk11aW1oUkxJT1lr?=
 =?utf-8?B?cDdEdjkzVzRQL2J4eW5IYlY3MmsrZDBkN0ZPa1VHQnVVVzRyZVZFN0dFcHlR?=
 =?utf-8?B?THFEdU0vVjFBQ0I2QnVKc3FlT1ZCbWNDSFo4NXJkWTFFU3REbmREZ05YS29U?=
 =?utf-8?B?U1NtODVneXZxa2g1di9raGhCUmlSN0d0OXU4aGhWV09jakZrS0NSYlk0SW9Q?=
 =?utf-8?B?TU5uWFEzSkh3Qzd2cmw1TzZrdVZjR1RDRWM5OFVXZWtDZzBwaWFOZDh1ckJ2?=
 =?utf-8?B?YzVhaUNKemNwaTdXa3U3Y0xyK2huNGJYWHNYQlRHbmNkSVdqZFZSd2R1RTJw?=
 =?utf-8?B?dzlHUGJUVmViOWJ1Y2Jad245eEZFOS9wS29vNG9MUjR0YUY2akFwVlVkanND?=
 =?utf-8?B?MVJzTmZOQ1FPMU5LU0ZmMmhxNE5VVkh1UG5pQ1RiQ05ldnl0T2thYWc4Q3E4?=
 =?utf-8?B?NnlKYUhsZ3RCWUdiU2NPVS8zcmRTdExkWEVSMDk1aXdIVzBXZzYzbnlhUTJR?=
 =?utf-8?B?bjE3MEpLZGhGeXhBTEVpemsvNDAxeFdmN1NydUQ3RElrOGJrMGtmWHJFTmY1?=
 =?utf-8?B?Q25hQ1JVMlF4ZytlQ1dvZksxaFJLOGxrUkp4ckxnbUdvRjFad2h0bWJKa3J6?=
 =?utf-8?B?blJBMHZQTTZXajdnNUdZMG05SEVQdWsrZW9zTHc2ZTZRY0lVUENTNS9tbCs5?=
 =?utf-8?B?UW5CWnlsSGIwODQzRCt0M0g4d0drT0NvdjV3MERLczVxdXNPdlhOQkRDRTFu?=
 =?utf-8?B?a1paeG9mN3RNMkNKWWozam03dVc0VUtRcXBnVHEwSGFLTVBYYUw0eExyUE5z?=
 =?utf-8?B?YnNxbzFoRDEwRy9SSVZwRGRrblRseC96dTBiNWpabUQxaGpYMTNhdERrWDJK?=
 =?utf-8?B?TkhsOHBVZEtBc3VVdDhpamlsTlRqMERIc3JLN2E3RDF0bFR6bjZTMlJhMVNN?=
 =?utf-8?B?U2VnVWpCQmNVaHE2cXhKV2d5NnFTRUNUc0U5bUdRb2ZKdzVsVXRpQUhrelNk?=
 =?utf-8?B?eUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7192f5c7-23dc-42c7-27c3-08daad373f35
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4855.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Oct 2022 16:23:22.2694
 (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: xSzS95qwewdK2PILvAmF0NeEOtjCWroOsgB7Exj382n4wPkDnG80bB1RaV3fbHxNiUf1kDQcKSmr0FZVChv0Ld1UIPsX4dr2TlLrHzJiUfI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR11MB5695
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_HI,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/12/2022 8:35 PM, Yao, Yuan wrote:
> 
> The reason is __copy_xstate_to_uabi_buf() copies data from &init_fpstate when the component
> is not existed in the source kernel fpstate (here is the AMX tile component), but the
> AMX TILE bit is removed from init_fpstate due to this patch, so the WARN is triggered and return
> NULL which causes kernel NULL pointer dereference later.

We have this in __copy_xstate_to_uabi_buf() [1]:

	mask = fpstate->user_xfeatures;

	for_each_extended_xfeature(i, mask) {
	...
	}

And the KVM code seems to set dynamic features regardless of the buffer 
reallocation [2]:

	vcpu->arch.guest_fpu.fpstate->user_xfeatures =
		vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE;

The kernel code seems to be aware of this as fpstate_realloc() does [3]:

	if (!guest_fpu)
		newfps->user_xfeatures = curfps->user_xfeatures | xfeatures;

But it updates the 'xfeature' bitmask for all:

	newfps->xfeatures = curfps->xfeatures | xfeatures;

So, I think we can do something like this here:

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c8340156bfd2..8ea7d0e95f1a 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1127,8 +1127,12 @@ void __copy_xstate_to_uabi_buf(struct membuf to, 
struct fpstate *fpstate,
          * non-compacted format disabled features still occupy state space,
          * but there is no state to copy from in the compacted
          * init_fpstate. The gap tracking will zero these states.
+        *
+        * In the case of guest fpstate, this user_xfeatures does not
+        * dynamically reflect the capacity of the XSAVE buffer but
+        * xfeatures does. So AND them together.
          */
-       mask = fpstate->user_xfeatures;
+       mask = fpstate->user_xfeatures & fpstate->xfeatures;

Let me also test this by running KVM.

Thanks,
Chang

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/xstate.c#n1131
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kvm/cpuid.c#n346
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/xstate.c#n1448