Re: [PATCH 1/2 v4] KVM: nSVM: Check reserved values for 'Type' and invalid vectors in EVENTINJ

[Sean Christopherson <seanjc@google.com>]

On Mon, Dec 07, 2020, Krish Sadhukhan wrote:
> According to sections "Canonicalization and Consistency Checks" and "Event
> Injection" in APM vol 2
> 
>     VMRUN exits with VMEXIT_INVALID error code if either:
>       - Reserved values of TYPE have been specified, or
>       - TYPE = 3 (exception) has been specified with a vector that does not
> 	correspond to an exception (this includes vector 2, which is an NMI,
> 	not an exception).
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>  arch/x86/include/asm/svm.h |  4 ++++
>  arch/x86/kvm/svm/nested.c  | 14 ++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 71d630bb5e08..d676f140cd19 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -341,9 +341,13 @@ struct vmcb {
>  #define SVM_EVTINJ_TYPE_MASK (7 << SVM_EVTINJ_TYPE_SHIFT)
>  
>  #define SVM_EVTINJ_TYPE_INTR (0 << SVM_EVTINJ_TYPE_SHIFT)
> +#define SVM_EVTINJ_TYPE_RESV1 (1 << SVM_EVTINJ_TYPE_SHIFT)
>  #define SVM_EVTINJ_TYPE_NMI (2 << SVM_EVTINJ_TYPE_SHIFT)
>  #define SVM_EVTINJ_TYPE_EXEPT (3 << SVM_EVTINJ_TYPE_SHIFT)
>  #define SVM_EVTINJ_TYPE_SOFT (4 << SVM_EVTINJ_TYPE_SHIFT)
> +#define SVM_EVTINJ_TYPE_RESV5 (5 << SVM_EVTINJ_TYPE_SHIFT)
> +#define SVM_EVTINJ_TYPE_RESV6 (6 << SVM_EVTINJ_TYPE_SHIFT)
> +#define SVM_EVTINJ_TYPE_RESV7 (7 << SVM_EVTINJ_TYPE_SHIFT)
>  
>  #define SVM_EVTINJ_VALID (1 << 31)
>  #define SVM_EVTINJ_VALID_ERR (1 << 11)
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 9e4c226dbf7d..fa51231c1f24 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -212,6 +212,9 @@ static bool svm_get_nested_state_pages(struct kvm_vcpu *vcpu)
>  
>  static bool nested_vmcb_check_controls(struct vmcb_control_area *control)
>  {
> +	u8 type, vector;
> +	bool valid;
> +
>  	if ((vmcb_is_intercept(control, INTERCEPT_VMRUN)) == 0)
>  		return false;
>  
> @@ -222,6 +225,17 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control)
>  	    !npt_enabled)
>  		return false;
>  
> +	valid = control->event_inj & SVM_EVTINJ_VALID;
> +	type = control->event_inj & SVM_EVTINJ_TYPE_MASK;

The mask is shifted by 8, which means type is guaranteed to be 0 since the value
will be truncated when casting to a u8.  I'm somewhat surprised neither
checkpatch nor checkpatch warns.  The types are also shifted, so the easiest
thing is probably to store it as a u32, same as event_inj.  I suspect your test
passes because type==0 is INTR and the test always injects #DE, which is likely
an illegal vector.

> +	if (valid && (type == SVM_EVTINJ_TYPE_RESV1 ||
> +	    type >= SVM_EVTINJ_TYPE_RESV5))
> +		return false;
> +
> +	vector = control->event_inj & SVM_EVTINJ_VEC_MASK;
> +	if (valid && (type == SVM_EVTINJ_TYPE_EXEPT))
> +		if (vector == NMI_VECTOR || vector > 31)

Preferred style is to combine these into a single statement.

> +			return false;
> +
>  	return true;
>  }
>  
> -- 
> 2.27.0
>