Re: [PATCH] VMX: Fix and improve guest state validity checks
On 05/13/2010 11:15 PM, Mohammed Gamal wrote: On Thu, May 13, 2010 at 9:24 AM, Avi Kivitya...@redhat.com wrote: On 05/11/2010 07:52 PM, Mohammed Gamal wrote: - Add 's' and 'g' field checks on segment registers - Correct SS checks for request and descriptor privilege levels Signed-off-by: Mohammed Gamalm.gamal...@gmail.com --- arch/x86/kvm/vmx.c | 73 +++ 1 files changed, 67 insertions(+), 6 deletions(-) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 777e00d..9805c2a 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -2121,16 +2121,30 @@ static bool stack_segment_valid(struct kvm_vcpu *vcpu) vmx_get_segment(vcpu,ss, VCPU_SREG_SS); ss_rpl = ss.selectorSELECTOR_RPL_MASK; - if (ss.unusable) + if (ss.dpl != ss_rpl) /* DPL != RPL */ + return false; + + if (ss.unusable) /* Short-circuit */ return true; If ss.unusable, do the dpl and rpl have any meaning? The idea is that dpl and rpl are checked on vmentry regardless of whether ss is usable or not. While the other checks are performed only if ss is usable. Any reference to back this up? I think rpl is valid regardless of ss.unusable (i.e. loading selector 0003 results in an unusable segment with rpl=3), but I don't see how dpl can be valid in an unusable segment. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VMX: Fix and improve guest state validity checks
On Tue, May 25, 2010 at 12:37 PM, Avi Kivity a...@redhat.com wrote: On 05/13/2010 11:15 PM, Mohammed Gamal wrote: On Thu, May 13, 2010 at 9:24 AM, Avi Kivitya...@redhat.com wrote: On 05/11/2010 07:52 PM, Mohammed Gamal wrote: - Add 's' and 'g' field checks on segment registers - Correct SS checks for request and descriptor privilege levels Signed-off-by: Mohammed Gamalm.gamal...@gmail.com --- arch/x86/kvm/vmx.c | 73 +++ 1 files changed, 67 insertions(+), 6 deletions(-) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 777e00d..9805c2a 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -2121,16 +2121,30 @@ static bool stack_segment_valid(struct kvm_vcpu *vcpu) vmx_get_segment(vcpu,ss, VCPU_SREG_SS); ss_rpl = ss.selector SELECTOR_RPL_MASK; - if (ss.unusable) + if (ss.dpl != ss_rpl) /* DPL != RPL */ + return false; + + if (ss.unusable) /* Short-circuit */ return true; If ss.unusable, do the dpl and rpl have any meaning? The idea is that dpl and rpl are checked on vmentry regardless of whether ss is usable or not. While the other checks are performed only if ss is usable. Any reference to back this up? I think rpl is valid regardless of ss.unusable (i.e. loading selector 0003 results in an unusable segment with rpl=3), but I don't see how dpl can be valid in an unusable segment. Intel 64 and IA-32 Architectures Software Developer’s Manual Volume 3B, System Programming Guide, Part 2, Chapter 22, Section 22.3.1.2: Checks on Guest Segment Registers. You'll note that DS, ES, FS, GS checks are done when the segment is usable. SS checks are not necessarily checked only when the segment is usable. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VMX: Fix and improve guest state validity checks
On 05/25/2010 01:36 PM, Mohammed Gamal wrote: Any reference to back this up? I think rpl is valid regardless of ss.unusable (i.e. loading selector 0003 results in an unusable segment with rpl=3), but I don't see how dpl can be valid in an unusable segment. Intel 64 and IA-32 Architectures Software Developer’s Manual Volume 3B, System Programming Guide, Part 2, Chapter 22, Section 22.3.1.2: Checks on Guest Segment Registers. You'll note that DS, ES, FS, GS checks are done when the segment is usable. SS checks are not necessarily checked only when the segment is usable. Strange, but consistent with If the unusable bit is 1, the base address, the segment limit, and the remainder of the access rights are undefined after VM entry. The only exceptions are the following: — Bits 3:0 of the base address for SS are cleared to 0. — SS.DPL: always loaded from the SS access-rights field. This will be the current privilege level (CPL) after the VM entry completes. — SS.B: set to 1. — The base addresses for FS and GS: always loaded. On processors that support Intel 64 architecture, the values loaded for base addresses for FS and GS are also manifest in the FS.base and GS.base MSRs. — The base address for LDTR on processors that support Intel 64 archi- tecture: set to an undefined but canonical value. — Bits 63:32 of the base addresses for SS, DS, and ES on processors that support Intel 64 architecture: cleared to 0. So you are right. Seems to me we can simplify vmx_get_cpl() on this basis to look at ss.dpl. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VMX: Fix and improve guest state validity checks
On 05/11/2010 07:52 PM, Mohammed Gamal wrote:
- Add 's' and 'g' field checks on segment registers
- Correct SS checks for request and descriptor privilege levels
Signed-off-by: Mohammed Gamalm.gamal...@gmail.com
---
arch/x86/kvm/vmx.c | 73 +++
1 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 777e00d..9805c2a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2121,16 +2121,30 @@ static bool stack_segment_valid(struct kvm_vcpu *vcpu)
vmx_get_segment(vcpu,ss, VCPU_SREG_SS);
ss_rpl = ss.selector SELECTOR_RPL_MASK;
- if (ss.unusable)
+ if (ss.dpl != ss_rpl) /* DPL != RPL */
+ return false;
+
+ if (ss.unusable) /* Short-circuit */
return true;
If ss.unusable, do the dpl and rpl have any meaning?
if (!ss.present)
return false;
+ if (ss.limit 0xfff0) {
+if ((ss.limit 0xfff) 0xfff)
+return false;
+if (!ss.g)
+return false;
+} else {
+if ((ss.limit 0xfff) == 0xfff)
+return false;
+if (ss.g)
+return false;
+}
There is no architectural way to break this. That is, without
virtualization, there is no way a real cpu will ever have a limit of
0x12345678.
We need to distinguish between big real mode and real mode that can be
virtualized using vm86, but we don't need to consider impossible setups.
@@ -2143,8 +2157,15 @@ static bool data_segment_valid(struct kvm_vcpu *vcpu,
int seg)
vmx_get_segment(vcpu,var, seg);
rpl = var.selector SELECTOR_RPL_MASK;
- if (var.unusable)
+ if (var.unusable) /* Short-circuit */
return true;
+ if (!(var.type AR_TYPE_ACCESSES_MASK))
+ return false;
Again, there is no architectural way for a segment not to have the
accessed bit set.
+ if (var.type AR_TYPE_CODE_MASK) {
+ if (!(var.type AR_TYPE_READABLE_MASK))
+ return false;
+ }
About this, I'm not sure.
+
if (!var.s)
return false;
if (!var.present)
@@ -2154,6 +2175,18 @@ static bool data_segment_valid(struct kvm_vcpu *vcpu,
int seg)
return false;
}
+ if (var.limit 0xfff0) {
+ if ((var.limit 0xfff) 0xfff)
+ return false;
+ if (!var.g)
+ return false;
+ } else {
+ if ((var.limit 0xfff) == 0xfff)
+ return false;
+ if (var.g)
+ return false;
+ }
Even disregarding the incorrectness, you shouldn't duplicate code like this.
@@ -2192,6 +2240,20 @@ static bool ldtr_valid(struct kvm_vcpu *vcpu)
return false;
if (!ldtr.present)
return false;
+ if (ldtr.s)
+ return false;
Architecturally impossible.
--
Do not meddle in the internals of kernels, for they are subtle and quick to
panic.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VMX: Fix and improve guest state validity checks
On Thu, May 13, 2010 at 9:24 AM, Avi Kivity a...@redhat.com wrote:
On 05/11/2010 07:52 PM, Mohammed Gamal wrote:
- Add 's' and 'g' field checks on segment registers
- Correct SS checks for request and descriptor privilege levels
Signed-off-by: Mohammed Gamalm.gamal...@gmail.com
---
arch/x86/kvm/vmx.c | 73
+++
1 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 777e00d..9805c2a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2121,16 +2121,30 @@ static bool stack_segment_valid(struct kvm_vcpu
*vcpu)
vmx_get_segment(vcpu,ss, VCPU_SREG_SS);
ss_rpl = ss.selector SELECTOR_RPL_MASK;
- if (ss.unusable)
+ if (ss.dpl != ss_rpl) /* DPL != RPL */
+ return false;
+
+ if (ss.unusable) /* Short-circuit */
return true;
If ss.unusable, do the dpl and rpl have any meaning?
The idea is that dpl and rpl are checked on vmentry regardless of
whether ss is usable or not. While the other checks are performed only
if ss is usable.
if (!ss.present)
return false;
+ if (ss.limit 0xfff0) {
+ if ((ss.limit 0xfff) 0xfff)
+ return false;
+ if (!ss.g)
+ return false;
+ } else {
+ if ((ss.limit 0xfff) == 0xfff)
+ return false;
+ if (ss.g)
+ return false;
+ }
There is no architectural way to break this. That is, without
virtualization, there is no way a real cpu will ever have a limit of
0x12345678.
We need to distinguish between big real mode and real mode that can be
virtualized using vm86, but we don't need to consider impossible setups.
I didn't realize this is architecturally impossible. I was simply
implementing the checks specified in the Intel manual. Now that we
know this is redunant, we can just drop these checks.
@@ -2143,8 +2157,15 @@ static bool data_segment_valid(struct kvm_vcpu
*vcpu, int seg)
vmx_get_segment(vcpu,var, seg);
rpl = var.selector SELECTOR_RPL_MASK;
- if (var.unusable)
+ if (var.unusable) /* Short-circuit */
return true;
+ if (!(var.type AR_TYPE_ACCESSES_MASK))
+ return false;
Again, there is no architectural way for a segment not to have the accessed
bit set.
+ if (var.type AR_TYPE_CODE_MASK) {
+ if (!(var.type AR_TYPE_READABLE_MASK))
+ return false;
+ }
About this, I'm not sure.
+
if (!var.s)
return false;
if (!var.present)
@@ -2154,6 +2175,18 @@ static bool data_segment_valid(struct kvm_vcpu
*vcpu, int seg)
return false;
}
+ if (var.limit 0xfff0) {
+ if ((var.limit 0xfff) 0xfff)
+ return false;
+ if (!var.g)
+ return false;
+ } else {
+ if ((var.limit 0xfff) == 0xfff)
+ return false;
+ if (var.g)
+ return false;
+ }
Even disregarding the incorrectness, you shouldn't duplicate code like this.
I was intending to consolidate it into a single function eventually, I
just wasn't sure that this was correct and I needed some comments on
it. It's not needed now anyhow.
@@ -2192,6 +2240,20 @@ static bool ldtr_valid(struct kvm_vcpu *vcpu)
return false;
if (!ldtr.present)
return false;
+ if (ldtr.s)
+ return false;
Architecturally impossible.
--
Do not meddle in the internals of kernels, for they are subtle and quick to
panic.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VMX: Fix and improve guest state validity checks
On Tue, May 11, 2010 at 07:52:41PM +0300, Mohammed Gamal wrote:
- Add 's' and 'g' field checks on segment registers
- Correct SS checks for request and descriptor privilege levels
Signed-off-by: Mohammed Gamal m.gamal...@gmail.com
---
arch/x86/kvm/vmx.c | 73 +++
1 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 777e00d..9805c2a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2121,16 +2121,30 @@ static bool stack_segment_valid(struct kvm_vcpu *vcpu)
vmx_get_segment(vcpu, ss, VCPU_SREG_SS);
ss_rpl = ss.selector SELECTOR_RPL_MASK;
- if (ss.unusable)
+ if (ss.dpl != ss_rpl) /* DPL != RPL */
+ return false;
+
+ if (ss.unusable) /* Short-circuit */
return true;
+
if (ss.type != 3 ss.type != 7)
return false;
if (!ss.s)
return false;
- if (ss.dpl != ss_rpl) /* DPL != RPL */
- return false;
if (!ss.present)
return false;
+ if (ss.limit 0xfff0) {
0x1fff limit and g==1 is valid, for example.
+if ((ss.limit 0xfff) 0xfff)
+return false;
+if (!ss.g)
+return false;
+} else {
+if ((ss.limit 0xfff) == 0xfff)
+return false;
!g segments can be up to 1Mbyte in size, and byte granular.
Please send as separate patches. Also, the limit checks could
be in a helper function since they are shared.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] VMX: Fix and improve guest state validity checks
- Add 's' and 'g' field checks on segment registers
- Correct SS checks for request and descriptor privilege levels
Signed-off-by: Mohammed Gamal m.gamal...@gmail.com
---
arch/x86/kvm/vmx.c | 73 +++
1 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 777e00d..9805c2a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2121,16 +2121,30 @@ static bool stack_segment_valid(struct kvm_vcpu *vcpu)
vmx_get_segment(vcpu, ss, VCPU_SREG_SS);
ss_rpl = ss.selector SELECTOR_RPL_MASK;
- if (ss.unusable)
+ if (ss.dpl != ss_rpl) /* DPL != RPL */
+ return false;
+
+ if (ss.unusable) /* Short-circuit */
return true;
+
if (ss.type != 3 ss.type != 7)
return false;
if (!ss.s)
return false;
- if (ss.dpl != ss_rpl) /* DPL != RPL */
- return false;
if (!ss.present)
return false;
+ if (ss.limit 0xfff0) {
+if ((ss.limit 0xfff) 0xfff)
+return false;
+if (!ss.g)
+return false;
+} else {
+if ((ss.limit 0xfff) == 0xfff)
+return false;
+if (ss.g)
+return false;
+}
+
return true;
}
@@ -2143,8 +2157,15 @@ static bool data_segment_valid(struct kvm_vcpu *vcpu,
int seg)
vmx_get_segment(vcpu, var, seg);
rpl = var.selector SELECTOR_RPL_MASK;
- if (var.unusable)
+ if (var.unusable) /* Short-circuit */
return true;
+ if (!(var.type AR_TYPE_ACCESSES_MASK))
+ return false;
+ if (var.type AR_TYPE_CODE_MASK) {
+ if (!(var.type AR_TYPE_READABLE_MASK))
+ return false;
+ }
+
if (!var.s)
return false;
if (!var.present)
@@ -2154,6 +2175,18 @@ static bool data_segment_valid(struct kvm_vcpu *vcpu,
int seg)
return false;
}
+ if (var.limit 0xfff0) {
+ if ((var.limit 0xfff) 0xfff)
+ return false;
+ if (!var.g)
+ return false;
+ } else {
+ if ((var.limit 0xfff) == 0xfff)
+ return false;
+ if (var.g)
+ return false;
+ }
+
/* TODO: Add other members to kvm_segment_field to allow checking for
other access
* rights flags
*/
@@ -2172,6 +2205,21 @@ static bool tr_valid(struct kvm_vcpu *vcpu)
return false;
if (tr.type != 3 tr.type != 11) /* TODO: Check if guest is in IA32e
mode */
return false;
+ if (tr.s)
+ return false;
+ if (tr.limit 0xfff0) {
+if ((tr.limit 0xfff) 0xfff)
+return false;
+if (!tr.g)
+return false;
+} else {
+if ((tr.limit 0xfff) == 0xfff)
+return false;
+if (tr.g)
+return false;
+}
+
+
if (!tr.present)
return false;
@@ -2184,7 +2232,7 @@ static bool ldtr_valid(struct kvm_vcpu *vcpu)
vmx_get_segment(vcpu, ldtr, VCPU_SREG_LDTR);
- if (ldtr.unusable)
+ if (ldtr.unusable) /* Short-circuit */
return true;
if (ldtr.selector SELECTOR_TI_MASK) /* TI = 1 */
return false;
@@ -2192,6 +2240,20 @@ static bool ldtr_valid(struct kvm_vcpu *vcpu)
return false;
if (!ldtr.present)
return false;
+ if (ldtr.s)
+ return false;
+ if (ldtr.limit 0xfff0) {
+if ((ldtr.limit 0xfff) 0xfff)
+return false;
+if (!ldtr.g)
+return false;
+} else {
+if ((ldtr.limit 0xfff) == 0xfff)
+return false;
+if (ldtr.g)
+return false;
+}
+
return true;
}
@@ -2251,7 +2313,6 @@ static bool guest_state_valid(struct kvm_vcpu *vcpu)
}
/* TODO:
* - Add checks on RIP
-* - Add checks on RFLAGS
*/
return true;
--
1.7.0.4
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
