Received-SPF: pass (google.com: domain of linux-kernel+bounces-169513-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <2f761cff-33f4-4434-a900-f87b6cf72016@intel.com>
Date: Mon, 6 May 2024 16:41:04 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v10 21/27] KVM: x86: Save and reload SSP to/from SMRAM
To: Sean Christopherson <seanjc@google.com>
CC: <pbonzini@redhat.com>, <dave.hansen@intel.com>, <x86@kernel.org>,
	<kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<peterz@infradead.org>, <chao.gao@intel.com>, <rick.p.edgecombe@intel.com>,
	<mlevitsk@redhat.com>, <john.allen@amd.com>
References: <20240219074733.122080-1-weijiang.yang@intel.com>
 <20240219074733.122080-22-weijiang.yang@intel.com>
 <ZjLHIwCsLoatrxQ4@google.com>
Content-Language: en-US
From: "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZjLHIwCsLoatrxQ4@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cThydmhaVUdhV1ZsRlBZTVlySnZXaGR3UkxYWm5CUGtnYmFoRXJKaklrTkNP?=
 =?utf-8?B?c0JsMmZhOTlZaTN0c1lld1MxTzhQcnhYL0Fsc28vSTFVWUwzaEg0RGZkeFBq?=
 =?utf-8?B?eHFKc0ZpVWFIU3htM2NjUXd1UnBGWnJ4T0VhWXMzem43ZVB1bVNMVjdhYmFS?=
 =?utf-8?B?dVdlaEF6ZzhqYzZDQnhDL3JIUHpWZWYrY3RtdE9YNWdRVzRNaDhDNTBna3o1?=
 =?utf-8?B?Z3RHUEdUNWs1cE1LaWlFVkkvQ1c0b1p0Y0syeEpOUUkyaHNsYXRKcDdwWUJO?=
 =?utf-8?B?dG5qYjZTSmgrYkVkMXJIdkRTV2FhOC9JaWpGdFBlRGEranBGSDI5MVdTOCtQ?=
 =?utf-8?B?SHJLZU5HQWdLZG1kdmpvVkIzSWhvQmM3aW0rUzduNlA4T0RueWsxMGpUKzBD?=
 =?utf-8?B?REcxL1ArMEJNc0E5WWhVZGtxS2hBT0R2TWVHS3lFRnE5NkFkTkpOMENUUHBk?=
 =?utf-8?B?R1lZMmZ0MzhWbHlCMjFzQzB2UWpWWldmV0xWeW8yY1dqNnpGTWFLQW9IamZG?=
 =?utf-8?B?VHhLOUEzSkVDZUF4UTV3Wm9zY0EvL1JJZjhvNHIrSDZOSG9aNG52WFBDSkc2?=
 =?utf-8?B?bkdMTEYwUzJVWVhMdkVOUmd4VFZ3cEdGV0tzSGhSeW0zaTcrY2VxTCtETi9h?=
 =?utf-8?B?QSt2ekpXRVY5UTA4S29aU0QxSHVjaGF5dkUzMTlzRmQ1ZDJqclFaMUZWL0pB?=
 =?utf-8?B?K0dCeXBWZ29HVTdsNVFxazB1Yjk1bCtEQkoreXg4cUdFeDRoUyt4V3o1V1E0?=
 =?utf-8?B?Y0lzQ0d3bGJCaE5LRmpnZUVvT3VhTS9rUFhRMDJJa1AzU1hwQUJVWmltTk5z?=
 =?utf-8?B?MDNEQWsyam9uenJsUlV6VStWWitzNEFYL1RmZUx1VnJsQ3JGV3RXNTF3UFdZ?=
 =?utf-8?B?aDF6c0MrRGtwb3UySXd0NjlCU1dHZVdvTGIvTm9TL0NFYjYreFpMMGVzRHp1?=
 =?utf-8?B?YUFPQko2Y1pXRWg0K05wWEppOTJtQ3cyNnRFSDg1VkkzdlFoM0k2em5TdjIr?=
 =?utf-8?B?b0dYSG9QMDNlSVo3S2RUWlEzcmRmTmFod0wzazRKUXZhZEw4cEt5emxqTi8z?=
 =?utf-8?B?aVhkdjRPSi9ERXBkK09hQk4xVjBlNzA4Z0QrTDhpbnhOUzVORk12aEVyY1Jp?=
 =?utf-8?B?OVR4bXZranJhZDBmaTVNQUpndDF0QmFhejBPcnkzSEJ2WmJGa3E0ZTFObzBB?=
 =?utf-8?B?VGZkQkhGaXVLN2ZISnNlTzIyWWVzWWg3Zm05T0p2OGxjU0RkYkJET1IwT2ZL?=
 =?utf-8?B?WTVtcG5UZFYvc2EvMXZWd3NoK2pQK1gzQkdMS1VDZG1nMmhMNjFBTkI5NWpi?=
 =?utf-8?B?TG5FSW9KbzRnaVA3RmRxOU1sSHFrT1hnTXFQVEpoMWRMWUthcUZSTTMyNDNw?=
 =?utf-8?B?ZEFRV085ZnJYNzhkSkNSa0VTUzNicDE1S1BraEZhTVIxZUVoR2RrWDFSMHNt?=
 =?utf-8?B?U29mSWlTRzl5L0J4cG02L2ZKaFE1UlRxRHJEODFIU3lSM3dQSUxTWmlnWC9Z?=
 =?utf-8?B?Z25UdDBkKzROQ0lWSWpvL2ptMFl0WDZNVzV5SmdKbDBCcHVHUFRUSTdsSkxy?=
 =?utf-8?B?b3dPaEdTZjZmNU1xMmNlMUYvaXR1OElEbmRoSG9HTEsrYVg3UW1tQUhrbkRF?=
 =?utf-8?B?T3RtSXJiTkszQjdIRWUrTmE1MElyZWNIb2pMY1dNMmlmK3pKdGNoT0UxUGZ3?=
 =?utf-8?B?cHdOSGIvUHBPaDZ6bjdRYXlPVERtSGdRTVRKVm5wdkpiZUsvR3dwd1ZnaE9L?=
 =?utf-8?B?bkl5UURpUXBMQTFJTVQxK1I3bklLRVZaZU5TSzNLTGx4OFF4ZFAxSTFkQ3kz?=
 =?utf-8?B?SitRQXlRL2thbDRRYzBIeXp0RzNlRDlRWGd5MWRFTzBtSDIrYjB3NTRTOUh3?=
 =?utf-8?B?WUp3VUxYcjdZdkRpOHVTUHZIdU5EclArZ1o1Sk95UlFUMFN2bloxbXB2b0xt?=
 =?utf-8?B?MXJvYzRjMVZsdThQVzZ0YWZRTFFFcTFSMk5YdlNTTUFKNmJkemZobERrSEN1?=
 =?utf-8?B?VjU3MnFsV3drVHNtV1NZQlNjQzF0MDhnaUtQT2cvTnE2UlFJdktFd1h5dlF3?=
 =?utf-8?B?d3poUGR4clRFRVJ2d0tvbkdUR0tzbXp6V2RxOW1WYXRFQVQzc1N4dmVpTEVK?=
 =?utf-8?B?aXFQUmlxZGM5K1hQUTRsNmF5TEhvSUVFcStCdDJVaEl5bGtiWFRoUTF5SmVN?=
 =?utf-8?B?bnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: dae693ce-be1a-4d9d-4e11-08dc6da848ff
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 May 2024 08:41:13.4423
 (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: VlAjodsmc/pslEFBqw8wefbjmUCnF7kPzSsBVGag0mQAWVIQA37rjZPp3buMgE4fwfK+XC5NIkfCIRJv+2jWog==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7880
X-OriginatorOrg: intel.com

On 5/2/2024 6:50 AM, Sean Christopherson wrote:
> On Sun, Feb 18, 2024, Yang Weijiang wrote:
>> Save CET SSP to SMRAM on SMI and reload it on RSM. KVM emulates HW arch
>> behavior when guest enters/leaves SMM mode,i.e., save registers to SMRAM
>> at the entry of SMM and reload them at the exit to SMM. Per SDM, SSP is
>> one of such registers on 64-bit Arch, and add the support for SSP. Note,
>> on 32-bit Arch, SSP is not defined in SMRAM, so fail 32-bit CET guest
>> launch.
>>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
>> ---
>>   arch/x86/kvm/cpuid.c | 11 +++++++++++
>>   arch/x86/kvm/smm.c   |  8 ++++++++
>>   arch/x86/kvm/smm.h   |  2 +-
>>   3 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 2bb1931103ad..c0e13040e35b 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -149,6 +149,17 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu,
>>   		if (vaddr_bits != 48 && vaddr_bits != 57 && vaddr_bits != 0)
>>   			return -EINVAL;
>>   	}
>> +	/*
>> +	 * Prevent 32-bit guest launch if shadow stack is exposed as SSP
>> +	 * state is not defined for 32-bit SMRAM.
> Why?  Lack of save/restore for SSP on 32-bit guests is a gap in Intel's
> architecture, I don't see why KVM should diverge from hardware.  I.e. just do
> nothing for SSP on SMI/RSM, because that's exactly what the architecture says
> will happen.

OK, will remove the check. I just wanted to avoid any undocumented hole if SHSTK is
exposed in CPUID.

>
>> +	 */
>> +	best = cpuid_entry2_find(entries, nent, 0x80000001,
>> +				 KVM_CPUID_INDEX_NOT_SIGNIFICANT);
>> +	if (best && !(best->edx & F(LM))) {
>> +		best = cpuid_entry2_find(entries, nent, 0x7, 0);
>> +		if (best && (best->ecx & F(SHSTK)))
>> +			return -EINVAL;
>> +	}
>>   
>>   	/*
>>   	 * Exposing dynamic xfeatures to the guest requires additional
>> diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c
>> index 45c855389ea7..7aac9c54c353 100644
>> --- a/arch/x86/kvm/smm.c
>> +++ b/arch/x86/kvm/smm.c
>> @@ -275,6 +275,10 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu,
>>   	enter_smm_save_seg_64(vcpu, &smram->gs, VCPU_SREG_GS);
>>   
>>   	smram->int_shadow = static_call(kvm_x86_get_interrupt_shadow)(vcpu);
>> +
>> +	if (guest_can_use(vcpu, X86_FEATURE_SHSTK))
>> +		KVM_BUG_ON(kvm_msr_read(vcpu, MSR_KVM_SSP, &smram->ssp),
>> +			   vcpu->kvm);
>>   }
>>   #endif
>>   
>> @@ -564,6 +568,10 @@ static int rsm_load_state_64(struct x86_emulate_ctxt *ctxt,
>>   	static_call(kvm_x86_set_interrupt_shadow)(vcpu, 0);
>>   	ctxt->interruptibility = (u8)smstate->int_shadow;
>>   
>> +	if (guest_can_use(vcpu, X86_FEATURE_SHSTK))
>> +		KVM_BUG_ON(kvm_msr_write(vcpu, MSR_KVM_SSP, smstate->ssp),
>> +			   vcpu->kvm);
>
> This should synthesize triple-fault, not WARN and kill the VM, as the value to
> be restored is guest controlled (the guest can scribble SMRAM from within the
> SMI handler).
>
> At that point, I would just synthesize triple-fault for the read path too.

Ah, yes, will fail with triple-fault in next version, thanks!

>