| Message ID | 20201207194129.7543-2-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nSVM: Check reserved values for 'Type' and invalid vectors in EVENTINJ | expand |
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 >
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.
On 11/12/20 22:44, Krish Sadhukhan wrote: >>> >>> + 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.
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; + 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) + return false; + return true; }
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(+)