linux-kernel - Re: [PATCH] KVM: nVMX: Sync L2 guest CET states between L1/L2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 11 Feb 2021 09:18:03 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Yang Weijiang <weijiang.yang@...el.com>
Cc:     pbonzini@...hat.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: nVMX: Sync L2 guest CET states between L1/L2

On Tue, Feb 09, 2021, Yang Weijiang wrote:
> When L2 guest status has been changed by L1 QEMU/KVM, sync the change back
> to L2 guest before the later's next vm-entry. On the other hand, if it's
> changed due to L2 guest, sync it back so as to let L1 guest see the change.
> 
> Signed-off-by: Yang Weijiang <weijiang.yang@...el.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 9728efd529a1..b9d8db8facea 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2602,6 +2602,12 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>  	/* Note: may modify VM_ENTRY/EXIT_CONTROLS and GUEST/HOST_IA32_EFER */
>  	vmx_set_efer(vcpu, vcpu->arch.efer);
>  
> +	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_CET_STATE) {
> +		vmcs_writel(GUEST_SSP, vmcs12->guest_ssp);
> +		vmcs_writel(GUEST_INTR_SSP_TABLE, vmcs12->guest_ssp_tbl);
> +		vmcs_writel(GUEST_S_CET, vmcs12->guest_s_cet);
> +	}
> +

This is incomplete.  If VM_ENTRY_LOAD_CET_STATE is not set, then CET state needs
to be propagated from vmcs01 to vmcs02.  See nested.vmcs01_debugctl and
nested.vmcs01_guest_bndcfgs.

It's tempting to say that we should add machinery to simplify implementing new
fields that are conditionally loading, e.g. define an array that specifies the
field, its control, and its offset in vmcs12, then process the array at the
appropriate time.  That might be overkill though...

>  	/*
>  	 * Guest state is invalid and unrestricted guest is disabled,
>  	 * which means L1 attempted VMEntry to L2 with invalid state.
> @@ -4152,6 +4158,12 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>  
>  	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_EFER)
>  		vmcs12->guest_ia32_efer = vcpu->arch.efer;
> +
> +	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_CET_STATE) {

This is wrong, guest state is saved on VM-Exit if the control is _supported_,
it doesn't have to be enabled.

  If the processor supports the 1-setting of the “load CET” VM-entry control,
  the contents of the IA32_S_CET and IA32_INTERRUPT_SSP_TABLE_ADDR MSRs are
  saved into the corresponding fields. On processors that do not support Intel
  64 architecture, bits 63:32 of these MSRs are not saved.

And I'm pretty sure we should define these fields as a so called "rare" fields,
i.e. add 'em to the case statement in is_vmcs12_ext_field() and process them in
sync_vmcs02_to_vmcs12_rare().  CET isn't easily emulated, so they should almost
never be read/written by a VMM, and thus aren't with synchronizing to vmcs12 on
every exit.

> +		vmcs12->guest_ssp = vmcs_readl(GUEST_SSP);
> +		vmcs12->guest_ssp_tbl = vmcs_readl(GUEST_INTR_SSP_TABLE);
> +		vmcs12->guest_s_cet = vmcs_readl(GUEST_S_CET);
> +	}
>  }
>  
>  /*
> -- 
> 2.26.2
>