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

[Krish Sadhukhan <krish.sadhukhan@oracle.com>]

On 12/7/20 12:17 PM, Sean Christopherson wrote:
> 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.


My bad. Will change it back to u32.

>
>> +	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.


The reason why I split them was to avoid using too many parentheses 
which was what Paolo was objecting to. 😁

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