* [PATCH 0/2 v4] KVM: nSVM: Check reserved values for 'Type' and invalid vectors in EVENTINJ
@ 2020-12-07 19:41 Krish Sadhukhan
2020-12-07 19:41 ` [PATCH 1/2 " Krish Sadhukhan
2020-12-07 19:41 ` [PATCH 2/2 v4] nSVM: Test " Krish Sadhukhan
0 siblings, 2 replies; 6+ messages in thread
From: Krish Sadhukhan @ 2020-12-07 19:41 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc
v3 -> v4:
1. Changed storage type of local variables, 'type' and 'vector' in
patch# 1, from u32 to u8.
2. Fixed build breakage due to missing parentheses around logical
operators. The second check has also been split into two 'if-else'
conditionals.
[PATCH 1/2 v4] KVM: nSVM: Check reserved values for 'Type' and invalid
[PATCH 2/2 v4] nSVM: Test reserved values for 'Type' and invalid vectors in
arch/x86/include/asm/svm.h | 4 ++++
arch/x86/kvm/svm/nested.c | 14 ++++++++++++++
2 files changed, 18 insertions(+)
Krish Sadhukhan (1):
KVM: nSVM: Check reserved values for 'Type' and invalid vectors in EVENTINJ
x86/svm_tests.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
Krish Sadhukhan (1):
nSVM: Test reserved values for 'Type' and invalid vectors in EVENTINJ
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2 v4] KVM: nSVM: Check reserved values for 'Type' and invalid vectors in EVENTINJ
2020-12-07 19:41 [PATCH 0/2 v4] KVM: nSVM: Check reserved values for 'Type' and invalid vectors in EVENTINJ Krish Sadhukhan
@ 2020-12-07 19:41 ` Krish Sadhukhan
2020-12-07 20:17 ` Sean Christopherson
2020-12-07 19:41 ` [PATCH 2/2 v4] nSVM: Test " Krish Sadhukhan
1 sibling, 1 reply; 6+ messages in thread
From: Krish Sadhukhan @ 2020-12-07 19:41 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc
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;
+ 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;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2 v4] nSVM: Test reserved values for 'Type' and invalid vectors in EVENTINJ
2020-12-07 19:41 [PATCH 0/2 v4] KVM: nSVM: Check reserved values for 'Type' and invalid vectors in EVENTINJ Krish Sadhukhan
2020-12-07 19:41 ` [PATCH 1/2 " Krish Sadhukhan
@ 2020-12-07 19:41 ` Krish Sadhukhan
1 sibling, 0 replies; 6+ messages in thread
From: Krish Sadhukhan @ 2020-12-07 19:41 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc
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).
Existing tests already cover part of the second rule. This patch covers the
the first rule and the missing pieces of the second rule.
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
x86/svm_tests.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index f78c9e4..b9be522 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -2132,6 +2132,43 @@ static void test_dr(void)
vmcb->save.dr7 = dr_saved;
}
+static void test_event_inject(void)
+{
+ u32 i;
+ u32 event_inj_saved = vmcb->control.event_inj;
+
+ handle_exception(DE_VECTOR, my_isr);
+
+ report (svm_vmrun() == SVM_EXIT_VMMCALL && count_exc == 0, "Test "
+ "No EVENTINJ");
+
+ /*
+ * Reserved values for 'Type' in EVENTINJ causes VMEXIT_INVALID.
+ */
+ for (i = 1; i < 8; i++) {
+ if (i != 1 && i < 5)
+ continue;
+ vmcb->control.event_inj = DE_VECTOR |
+ i << SVM_EVTINJ_TYPE_SHIFT | SVM_EVTINJ_VALID;
+ report(svm_vmrun() == SVM_EXIT_ERR && count_exc == 0,
+ "Test invalid TYPE (%x) in EVENTINJ", i);
+ }
+
+ /*
+ * Invalid vector number for event type 'exception' in EVENTINJ
+ * causes VMEXIT_INVALID.
+ */
+ for (i = 32; i < 256; i += 4) {
+ vmcb->control.event_inj = i | SVM_EVTINJ_TYPE_EXEPT |
+ SVM_EVTINJ_VALID;
+ report(svm_vmrun() == SVM_EXIT_ERR && count_exc == 0,
+ "Test invalid vector (%u) in EVENTINJ for event type "
+ "\'exception\'", i);
+ }
+
+ vmcb->control.event_inj = event_inj_saved;
+}
+
static void svm_guest_state_test(void)
{
test_set_guest(basic_guest_main);
@@ -2141,6 +2178,7 @@ static void svm_guest_state_test(void)
test_cr3();
test_cr4();
test_dr();
+ test_event_inject();
}
struct svm_test svm_tests[] = {
--
2.18.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2 v4] KVM: nSVM: Check reserved values for 'Type' and invalid vectors in EVENTINJ
2020-12-07 19:41 ` [PATCH 1/2 " Krish Sadhukhan
@ 2020-12-07 20:17 ` Sean Christopherson
2020-12-11 21:44 ` Krish Sadhukhan
0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2020-12-07 20:17 UTC (permalink / raw)
To: Krish Sadhukhan; +Cc: kvm, pbonzini, jmattson
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2 v4] KVM: nSVM: Check reserved values for 'Type' and invalid vectors in EVENTINJ
2020-12-07 20:17 ` Sean Christopherson
@ 2020-12-11 21:44 ` Krish Sadhukhan
2020-12-11 21:49 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Krish Sadhukhan @ 2020-12-11 21:44 UTC (permalink / raw)
To: Sean Christopherson; +Cc: kvm, pbonzini, jmattson
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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2 v4] KVM: nSVM: Check reserved values for 'Type' and invalid vectors in EVENTINJ
2020-12-11 21:44 ` Krish Sadhukhan
@ 2020-12-11 21:49 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-12-11 21:49 UTC (permalink / raw)
To: Krish Sadhukhan, Sean Christopherson; +Cc: kvm, jmattson
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. 😁
But you kept the parentheses that I was objecting to. Good:
if (valid && type == SVM_EVTINJ_TYPE_EXEPT &&
(vector == NMI_VECTOR || vector > 31))
Bad:
if (valid && (type == SVM_EVTINJ_TYPE_EXEPT) &&
(vector == NMI_VECTOR || vector > 31))
There should be no parentheses around relational operators, only around
& | ^ and && ||.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-11 22:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-07 19:41 [PATCH 0/2 v4] KVM: nSVM: Check reserved values for 'Type' and invalid vectors in EVENTINJ Krish Sadhukhan
2020-12-07 19:41 ` [PATCH 1/2 " Krish Sadhukhan
2020-12-07 20:17 ` Sean Christopherson
2020-12-11 21:44 ` Krish Sadhukhan
2020-12-11 21:49 ` Paolo Bonzini
2020-12-07 19:41 ` [PATCH 2/2 v4] nSVM: Test " Krish Sadhukhan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.