* [PATCH] VMX: Fix and improve guest state validity checks
@ 2010-05-11 16:52 Mohammed Gamal
2010-05-13 1:52 ` Marcelo Tosatti
2010-05-13 6:24 ` Avi Kivity
0 siblings, 2 replies; 7+ messages in thread
From: Mohammed Gamal @ 2010-05-11 16:52 UTC (permalink / raw)
To: avi; +Cc: mtosatti, kvm, Mohammed Gamal
- Add 's' and 'g' field checks on segment registers
- Correct SS checks for request and descriptor privilege levels
Signed-off-by: Mohammed Gamal <m.gamal005@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 & 0xfff00000) {
+ 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 & 0xfff00000) {
+ 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 & 0xfff00000) {
+ 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 & 0xfff00000) {
+ 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] VMX: Fix and improve guest state validity checks
2010-05-11 16:52 [PATCH] VMX: Fix and improve guest state validity checks Mohammed Gamal
@ 2010-05-13 1:52 ` Marcelo Tosatti
2010-05-13 6:24 ` Avi Kivity
1 sibling, 0 replies; 7+ messages in thread
From: Marcelo Tosatti @ 2010-05-13 1:52 UTC (permalink / raw)
To: Mohammed Gamal; +Cc: avi, kvm
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.gamal005@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 & 0xfff00000) {
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] VMX: Fix and improve guest state validity checks
2010-05-11 16:52 [PATCH] VMX: Fix and improve guest state validity checks Mohammed Gamal
2010-05-13 1:52 ` Marcelo Tosatti
@ 2010-05-13 6:24 ` Avi Kivity
2010-05-13 20:15 ` Mohammed Gamal
1 sibling, 1 reply; 7+ messages in thread
From: Avi Kivity @ 2010-05-13 6:24 UTC (permalink / raw)
To: Mohammed Gamal; +Cc: mtosatti, kvm
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 Gamal<m.gamal005@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& 0xfff00000) {
> + 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& 0xfff00000) {
> + 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] VMX: Fix and improve guest state validity checks
2010-05-13 6:24 ` Avi Kivity
@ 2010-05-13 20:15 ` Mohammed Gamal
2010-05-25 9:37 ` Avi Kivity
0 siblings, 1 reply; 7+ messages in thread
From: Mohammed Gamal @ 2010-05-13 20:15 UTC (permalink / raw)
To: Avi Kivity; +Cc: mtosatti, kvm
On Thu, May 13, 2010 at 9:24 AM, Avi Kivity <avi@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 Gamal<m.gamal005@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& 0xfff00000) {
>> + 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& 0xfff00000) {
>> + 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.
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] VMX: Fix and improve guest state validity checks
2010-05-13 20:15 ` Mohammed Gamal
@ 2010-05-25 9:37 ` Avi Kivity
2010-05-25 10:36 ` Mohammed Gamal
0 siblings, 1 reply; 7+ messages in thread
From: Avi Kivity @ 2010-05-25 9:37 UTC (permalink / raw)
To: Mohammed Gamal; +Cc: mtosatti, kvm
On 05/13/2010 11:15 PM, Mohammed Gamal wrote:
> On Thu, May 13, 2010 at 9:24 AM, Avi Kivity<avi@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 Gamal<m.gamal005@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.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] VMX: Fix and improve guest state validity checks
2010-05-25 9:37 ` Avi Kivity
@ 2010-05-25 10:36 ` Mohammed Gamal
2010-05-25 11:22 ` Avi Kivity
0 siblings, 1 reply; 7+ messages in thread
From: Mohammed Gamal @ 2010-05-25 10:36 UTC (permalink / raw)
To: Avi Kivity; +Cc: mtosatti, kvm
On Tue, May 25, 2010 at 12:37 PM, Avi Kivity <avi@redhat.com> wrote:
> On 05/13/2010 11:15 PM, Mohammed Gamal wrote:
>>
>> On Thu, May 13, 2010 at 9:24 AM, Avi Kivity<avi@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 Gamal<m.gamal005@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
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] VMX: Fix and improve guest state validity checks
2010-05-25 10:36 ` Mohammed Gamal
@ 2010-05-25 11:22 ` Avi Kivity
0 siblings, 0 replies; 7+ messages in thread
From: Avi Kivity @ 2010-05-25 11:22 UTC (permalink / raw)
To: Mohammed Gamal; +Cc: mtosatti, kvm
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-05-25 11:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-11 16:52 [PATCH] VMX: Fix and improve guest state validity checks Mohammed Gamal
2010-05-13 1:52 ` Marcelo Tosatti
2010-05-13 6:24 ` Avi Kivity
2010-05-13 20:15 ` Mohammed Gamal
2010-05-25 9:37 ` Avi Kivity
2010-05-25 10:36 ` Mohammed Gamal
2010-05-25 11:22 ` Avi Kivity
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox