Xen devel - Apr 2012 - [PATCH] nested vmx: fix instruction decode segment limit check

- no limit check in 64-bit mode (is not special in any way)
- limit check is needed in compatibility mode
- canonical address check should instead be performed in 64-bit mode
- the last accessed byte must be within limits, not the first byte past
  the accessed range

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -319,7 +319,7 @@ static int decode_vmx_inst(struct cpu_us
 {
     struct vcpu *v = current;
     union vmx_inst_info info;
-    struct segment_register seg;
+    struct segment_register seg, cs;
     unsigned long base, index, seg_base, disp, offset;
     int scale, size;
 
@@ -342,6 +342,11 @@ static int decode_vmx_inst(struct cpu_us
         hvm_get_segment_register(v, sreg_to_index[info.fields.segment],
&seg);
         seg_base = seg.base;
 
+        if ( hvm_long_mode_enabled(v) )
+            hvm_get_segment_register(v, x86_seg_cs, &cs);
+        else
+            memset(&cs, 0, sizeof(cs));
+
         base = info.fields.base_reg_invalid ? 0 :
             reg_read(regs, info.fields.base_reg);
 
@@ -355,8 +360,10 @@ static int decode_vmx_inst(struct cpu_us
         size = 1 << (info.fields.addr_size + 1);
 
         offset = base + index * scale + disp;
-        if ( (offset > seg.limit || offset + size > seg.limit) &&
-            (!hvm_long_mode_enabled(v) || info.fields.segment == VMX_SREG_GS) )
+        if ( cs.attr.fields.l ?
+             (!is_canonical_address(offset) ||
+              !is_canonical_address(offset + size - 1)) :
+             (offset + size - 1 < offset || offset + size - 1 >
seg.limit) )
             goto gp_fault;
 
         if ( poperandS != NULL &&





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Hi, 

At 16:17 +0100 on 24 Apr (1335284232), Jan Beulich
wrote:
> - no limit check in 64-bit mode (is not special in any way)
> - limit check is needed in compatibility mode
> - canonical address check should instead be performed in 64-bit mode
> - the last accessed byte must be within limits, not the first byte past
>   the accessed range
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -319,7 +319,7 @@ static int decode_vmx_inst(struct cpu_us
>  {
>      struct vcpu *v = current;
>      union vmx_inst_info info;
> -    struct segment_register seg;
> +    struct segment_register seg, cs;
>      unsigned long base, index, seg_base, disp, offset;
>      int scale, size;
>  
> @@ -342,6 +342,11 @@ static int decode_vmx_inst(struct cpu_us
>          hvm_get_segment_register(v, sreg_to_index[info.fields.segment],
&seg);
>          seg_base = seg.base;
>  
> +        if ( hvm_long_mode_enabled(v) )
> +            hvm_get_segment_register(v, x86_seg_cs, &cs);
> +        else
> +            memset(&cs, 0, sizeof(cs));
> +
I found this a bit confusing - maybe you could extract the attr.fields.l
bit into a bool here instead of zeroing the struct and extracting it later?

Cheers,

Tim.

>>> On 24.04.12 at 19:24, Tim Deegan <tim@xen.org> wrote:
>> @@ -342,6 +342,11 @@ static int decode_vmx_inst(struct cpu_us
>>          hvm_get_segment_register(v,
sreg_to_index[info.fields.segment], &seg);
>>          seg_base = seg.base;
>>  
>> +        if ( hvm_long_mode_enabled(v) )
>> +            hvm_get_segment_register(v, x86_seg_cs, &cs);
>> +        else
>> +            memset(&cs, 0, sizeof(cs));
>> +
> 
> I found this a bit confusing - maybe you could extract the attr.fields.l
> bit into a bool here instead of zeroing the struct and extracting it later?
Okay, let me re-do that, and fold in some other adjustments I thought
of making after having sent out the patch.

Jan