Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Fri, 1 Dec 2023 14:33:15 +0800
From:   Chao Gao <chao.gao@intel.com>
To:     Maxim Levitsky <mlevitsk@redhat.com>
CC:     Yang Weijiang <weijiang.yang@intel.com>, <seanjc@google.com>,
        <pbonzini@redhat.com>, <dave.hansen@intel.com>,
        <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <peterz@infradead.org>, <rick.p.edgecombe@intel.com>,
        <john.allen@amd.com>
Subject: Re: [PATCH v7 22/26] KVM: VMX: Set up interception for CET MSRs
Message-ID: <ZWl+K55yUaCLCtqw@chao-email>
References: <20231124055330.138870-1-weijiang.yang@intel.com>
 <20231124055330.138870-23-weijiang.yang@intel.com>
 <393d82243b7f44731439717be82b20fbeda45c77.camel@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <393d82243b7f44731439717be82b20fbeda45c77.camel@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?rotE9RY4q7t2+YN5sgZgSaRYgcpgoZalC0/07/x2n1snJdcmfqLIKqQv8IHK?=
 =?us-ascii?Q?JtyKkp7K7s6D/wzvaSeutm5/QT+7Mtc6mpdK6xyvAkNy0Fp6JMSo6hjOYZZb?=
 =?us-ascii?Q?CrLqiKVgLRMn5ICSagNsapUyB0TYgaFDjQTzYvSolPGgGv5AWyMFIksTkP/N?=
 =?us-ascii?Q?H8my9ildCqKj4lwJfmHZdwXZUiTbUAI80wmE632Z1RXF6gAcfNmrZBP6GcN8?=
 =?us-ascii?Q?zKyS3svYxrThVgkIq5QUvsooOxioj7xgmMI4lAASwt09oHh5LTV/gKtZCBo8?=
 =?us-ascii?Q?kmy5zsiYLO+fD0TutQx0p9h7Ga/GO3YOxSpHCBP0T0SLcZHxVZiWIa6eQ408?=
 =?us-ascii?Q?pB9tOtNzZa/1+GWIISsDfvP9Q4h3TRXi+HWP0VJ0AaQSGvhXSLf1Nd2NH37T?=
 =?us-ascii?Q?jFQGeoQi3YbjR+sNbDaXdZmMdC6Y2xLZF8wnR5p/1xoeytRDvge30+olM+03?=
 =?us-ascii?Q?uLaCv2NFdckPPRDo8lVM8nJgv6PttvkLe86W19n1DlWkC/RlZjDWEEZ7Om9H?=
 =?us-ascii?Q?MqMuNEvbwvZ6ZmOYMzWkEkGQbr5hJfQWPO2Q3exYk5Vz17qPtqodHjjhMnHg?=
 =?us-ascii?Q?ku4mkCpMyCnUy8t7kSHahl/MXxZnIKkFxBzfSTb4rAykr4OwTS6deQBTgcx/?=
 =?us-ascii?Q?wzNQqggMr71ZDY7xLrAaDb54rDVYbBCO6Z2WlqkKCVJeTbFEesmvQHHjzMtK?=
 =?us-ascii?Q?zTikwucNgsdPrVH5QDlSY9kmdBHKN57ZKYy+6lraN/rqda1RRjcSdZAh7qOb?=
 =?us-ascii?Q?vlJhy0elk3oftRFQseH3owVUYrMN+7k1YvmSIgRxDV7sh/y43uW0562hi8Yu?=
 =?us-ascii?Q?Y+l9LsVWcRERKtnxAUruEKhzXQhnI0W3utAxIfqrZPlT1h0VE54pOzHylUej?=
 =?us-ascii?Q?5CGni/hqUA1H74XMWqVGLMYIDviPmHfojjZGNEUZ/ElGmftkentAxY8pjxSt?=
 =?us-ascii?Q?FbXzOy4vVcm69Vq4MvpsMDuTGME3qsFrGlzxIlB5ltuvV/2FkS4F699MR3cl?=
 =?us-ascii?Q?GhNLXGO/AaRz8jCCB6XEMJcvBICNXvsK5tFjlg7CwdNBukL135nxtCX9Mj5R?=
 =?us-ascii?Q?i47jOP73VldD+grazXclQc06rE/mcZsT+MlCWPUdVLtjMaihmeMzttMnIqT3?=
 =?us-ascii?Q?YnaOcFisF9qSlLqjsJ2QY7ownO/QqBdp5UnQGNLM6hwnVCyKolNFxm8J/OwA?=
 =?us-ascii?Q?7xuLRqimll7Xm1KTbgmnkrh1TeEFrHERMyJcm2/d31UJl06L2m5ZVUjKYSxb?=
 =?us-ascii?Q?Ie6nVJ2nkOr8o2S3a0uX2D7lHKDE1vpVTZuowwOmhhg8KS5sKJGD3qRrWNJV?=
 =?us-ascii?Q?NR7e4QoU2tVHCEwVB7DmDNFlfUzpKGaAnydLYC4BslFBZxrkMDfRZJTwgRG1?=
 =?us-ascii?Q?AOB6P11duGbKaeqeY2rG7Hh4R3Q8ZvsPOyl0Aku3LppOMYSD9M/Bi6HSBeKD?=
 =?us-ascii?Q?kbg/FJ7FLdrP8jtRw+7NmXDFgx0PbVuIVHWas7pjY2p3GANXkyQY9SIQj3Iv?=
 =?us-ascii?Q?3CFoNXrsoe9j43L5UZyY9hi4qRC0hzlWzL/MLwTESSDZ1uoc3Mucyy12VQ8T?=
 =?us-ascii?Q?0oRNRWGGsGftvk6t13UtcmrW88sXEYYLN15Nfv0l?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 815d61d2-8ccd-4b45-5414-08dbf2376b55
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Dec 2023 06:33:25.2631
 (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: l+g5lNLxEsc4srBRcADN0AL0PGhTiHOYSe6I6uSoqyk0VYIgqJfwUTKtQ380BFF6KHdbi+Km/8lvAEmLedmBsQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB8208
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Thu, 30 Nov 2023 22:34:06 -0800 (PST)

On Thu, Nov 30, 2023 at 07:44:45PM +0200, Maxim Levitsky wrote:
>On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote:
>> Enable/disable CET MSRs interception per associated feature configuration.
>> Shadow Stack feature requires all CET MSRs passed through to guest to make
>> it supported in user and supervisor mode while IBT feature only depends on
>> MSR_IA32_{U,S}_CETS_CET to enable user and supervisor IBT.
>> 
>> Note, this MSR design introduced an architectural limitation of SHSTK and
>> IBT control for guest, i.e., when SHSTK is exposed, IBT is also available
>> to guest from architectual perspective since IBT relies on subset of SHSTK
>> relevant MSRs.
>> 
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>>  arch/x86/kvm/vmx/vmx.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>> 
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 554f665e59c3..e484333eddb0 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -699,6 +699,10 @@ static bool is_valid_passthrough_msr(u32 msr)
>>  	case MSR_LBR_CORE_TO ... MSR_LBR_CORE_TO + 8:
>>  		/* LBR MSRs. These are handled in vmx_update_intercept_for_lbr_msrs() */
>>  		return true;
>> +	case MSR_IA32_U_CET:
>> +	case MSR_IA32_S_CET:
>> +	case MSR_IA32_PL0_SSP ... MSR_IA32_INT_SSP_TAB:
>> +		return true;
>>  	}
>>  
>>  	r = possible_passthrough_msr_slot(msr) != -ENOENT;
>> @@ -7766,6 +7770,42 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
>>  		vmx->pt_desc.ctl_bitmask &= ~(0xfULL << (32 + i * 4));
>>  }
>>  
>> +static void vmx_update_intercept_for_cet_msr(struct kvm_vcpu *vcpu)
>> +{
>> +	bool incpt;
>> +
>> +	if (kvm_cpu_cap_has(X86_FEATURE_SHSTK)) {
>> +		incpt = !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
>> +
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_U_CET,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_S_CET,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL0_SSP,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL1_SSP,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL2_SSP,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL3_SSP,
>> +					  MSR_TYPE_RW, incpt);
>> +		if (guest_cpuid_has(vcpu, X86_FEATURE_LM))
>> +			vmx_set_intercept_for_msr(vcpu, MSR_IA32_INT_SSP_TAB,
>> +						  MSR_TYPE_RW, incpt);
>> +		if (!incpt)
>> +			return;
>> +	}
>> +
>> +	if (kvm_cpu_cap_has(X86_FEATURE_IBT)) {
>> +		incpt = !guest_cpuid_has(vcpu, X86_FEATURE_IBT);
>> +
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_U_CET,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_S_CET,
>> +					  MSR_TYPE_RW, incpt);
>> +	}
>> +}
>> +
>>  static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>>  {
>>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>> @@ -7843,6 +7883,8 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>>  
>>  	/* Refresh #PF interception to account for MAXPHYADDR changes. */
>>  	vmx_update_exception_bitmap(vcpu);
>> +
>> +	vmx_update_intercept_for_cet_msr(vcpu);
>>  }
>>  
>>  static u64 vmx_get_perf_capabilities(void)
>
>My review feedback from the previous patch still applies as well,
>
>I still think that we should either try a best effort approach to plug
>this virtualization hole, or we at least should fail guest creation
>if the virtualization hole is present as I said:
>

>"Another, much simpler option is to fail the guest creation if the shadow stack + indirect branch tracking
>state differs between host and the guest, unless both are disabled in the guest.
>(in essence don't let the guest be created if (2) or (3) happen)"

Enforcing a "none" or "all" policy is a temporary solution. in future, if some
reserved bits in S/U_CET MSRs are extended for new features, there will be:

	platform A supports SS + IBT
	platform B supports SS + IBT + new feature

Guests running on B inevitably have the same virtualization hole. and if kvm
continues enforcing the policy on B, then VM migration from A to B would be
impossible.

To me, intercepting S/U_CET MSR and CET_S/U xsave components is intricate and
yields marginal benefits. And I also doubt any reasonable OS implementation
would depend on #GP of WRMSR to S/U_CET MSRs for functionalities. So, I vote
to leave the patch as-is.

>
>Please at least tell me what do you think about this.

>
>Best regards,
>	Maxim Levitsky
>
>
>