On Thu, May 13, 2010 at 9:24 AM, Avi Kivity <avi@xxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
