kvm_get_msrs() adds U_CET and S_CET when CET shadow stacks or IBT are
enabled, but it checks FEAT_7_0_EDX with the ECX-defined
CPUID_7_0_ECX_CET_SHSTK bit before adding the PL0-PL3 SSP MSRs.

CPUID_7_0_ECX_CET_SHSTK belongs to FEAT_7_0_ECX. The current check
therefore skips MSR_IA32_PL0_SSP through MSR_IA32_PL3_SSP even when
guest shadow stacks are enabled.

kvm_put_msrs() already uses FEAT_7_0_ECX for the same shadow-stack
condition. Use the same feature word in kvm_get_msrs() so QEMU does
not restore CET enablement with stale or zero SSP values.

Fixes: b6f85c5e4527 ("i386/kvm: Add save/restore support for CET MSRs")
Cc: qemu-stable@nongnu.org
Signed-off-by: Saul Freedman <fre3dm4n@gmail.com>
---
 target/i386/kvm/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 9e35288..3f4a241 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5021,7 +5021,7 @@ static int kvm_get_msrs(X86CPU *cpu)
         kvm_msr_entry_add(cpu, MSR_IA32_U_CET, 0);
         kvm_msr_entry_add(cpu, MSR_IA32_S_CET, 0);
 
-        if (env->features[FEAT_7_0_EDX] & CPUID_7_0_ECX_CET_SHSTK) {
+        if (env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_CET_SHSTK) {
             kvm_msr_entry_add(cpu, MSR_IA32_PL0_SSP, 0);
             kvm_msr_entry_add(cpu, MSR_IA32_PL1_SSP, 0);
             kvm_msr_entry_add(cpu, MSR_IA32_PL2_SSP, 0);
-- 
2.39.5

On 15/6/26 05:33, Saul Freedman wrote:
> kvm_get_msrs() adds U_CET and S_CET when CET shadow stacks or IBT are
> enabled, but it checks FEAT_7_0_EDX with the ECX-defined
> CPUID_7_0_ECX_CET_SHSTK bit before adding the PL0-PL3 SSP MSRs.
> 
> CPUID_7_0_ECX_CET_SHSTK belongs to FEAT_7_0_ECX. The current check
> therefore skips MSR_IA32_PL0_SSP through MSR_IA32_PL3_SSP even when
> guest shadow stacks are enabled.
> 
> kvm_put_msrs() already uses FEAT_7_0_ECX for the same shadow-stack
> condition. Use the same feature word in kvm_get_msrs() so QEMU does
> not restore CET enablement with stale or zero SSP values.
> 
> Fixes: b6f85c5e4527 ("i386/kvm: Add save/restore support for CET MSRs")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Saul Freedman <fre3dm4n@gmail.com>
> ---
>   target/i386/kvm/kvm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 9e35288..3f4a241 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5021,7 +5021,7 @@ static int kvm_get_msrs(X86CPU *cpu)
>           kvm_msr_entry_add(cpu, MSR_IA32_U_CET, 0);
>           kvm_msr_entry_add(cpu, MSR_IA32_S_CET, 0);
>   
> -        if (env->features[FEAT_7_0_EDX] & CPUID_7_0_ECX_CET_SHSTK) {
> +        if (env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_CET_SHSTK) {
>               kvm_msr_entry_add(cpu, MSR_IA32_PL0_SSP, 0);
>               kvm_msr_entry_add(cpu, MSR_IA32_PL1_SSP, 0);
>               kvm_msr_entry_add(cpu, MSR_IA32_PL2_SSP, 0);

I note this huge function could be made more readable using
various has_feature booleans, such:

   bool has_cet_shstk = env->features[FEAT_7_0_EDX]
                        & CPUID_7_0_ECX_CET_SHSTK;
   if (has_cet_shstk || ...) {

Anyhow this patch looks correct, so:
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>