Jan Beulich
2013-Sep-20 13:07 UTC
[PATCH] x86/HVM: linear address must be canonical for the whole accessed range
... rather than just for the first byte.
While at it, also
- make the real mode case at least dpo a wrap around check
- drop the mis-named "gpf" label (we''re not generating faults
here)
and use in-place returns instead
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1931,8 +1931,7 @@ int hvm_virtual_to_linear_addr(
unsigned int addr_size,
unsigned long *linear_addr)
{
- unsigned long addr = offset;
- uint32_t last_byte;
+ unsigned long addr = offset, last_byte;
if ( !(current->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE) )
{
@@ -1941,6 +1940,9 @@ int hvm_virtual_to_linear_addr(
* Certain of them are not done in native real mode anyway.
*/
addr = (uint32_t)(addr + reg->base);
+ last_byte = (uint32_t)addr + bytes - 1;
+ if ( last_byte < addr )
+ return 0;
}
else if ( addr_size != 64 )
{
@@ -1952,17 +1954,17 @@ int hvm_virtual_to_linear_addr(
{
case hvm_access_read:
if ( (reg->attr.fields.type & 0xa) == 0x8 )
- goto gpf; /* execute-only code segment */
+ return 0; /* execute-only code segment */
break;
case hvm_access_write:
if ( (reg->attr.fields.type & 0xa) != 0x2 )
- goto gpf; /* not a writable data segment */
+ return 0; /* not a writable data segment */
break;
default:
break;
}
- last_byte = offset + bytes - 1;
+ last_byte = (uint32_t)offset + bytes - 1;
/* Is this a grows-down data segment? Special limit check if so. */
if ( (reg->attr.fields.type & 0xc) == 0x4 )
@@ -1973,10 +1975,10 @@ int hvm_virtual_to_linear_addr(
/* Check first byte and last byte against respective bounds. */
if ( (offset <= reg->limit) || (last_byte < offset) )
- goto gpf;
+ return 0;
}
else if ( (last_byte > reg->limit) || (last_byte < offset) )
- goto gpf; /* last byte is beyond limit or wraps 0xFFFFFFFF */
+ return 0; /* last byte is beyond limit or wraps 0xFFFFFFFF */
/*
* Hardware truncates to 32 bits in compatibility mode.
@@ -1993,15 +1995,14 @@ int hvm_virtual_to_linear_addr(
if ( (seg == x86_seg_fs) || (seg == x86_seg_gs) )
addr += reg->base;
- if ( !is_canonical_address(addr) )
- goto gpf;
+ last_byte = addr + bytes - 1;
+ if ( !is_canonical_address(addr) || last_byte < addr ||
+ !is_canonical_address(last_byte) )
+ return 0;
}
*linear_addr = addr;
return 1;
-
- gpf:
- return 0;
}
/* On non-NULL return, we leave this function holding an additional
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Keir Fraser
2013-Sep-20 15:30 UTC
Re: [PATCH] x86/HVM: linear address must be canonical for the whole accessed range
On 20/09/2013 14:07, "Jan Beulich" <JBeulich@suse.com> wrote:> ... rather than just for the first byte. > > While at it, also > - make the real mode case at least dpo a wrap around check > - drop the mis-named "gpf" label (we''re not generating faults here) > and use in-place returns instead > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Acked-by: Keir Fraser <keir@xen.org>> --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -1931,8 +1931,7 @@ int hvm_virtual_to_linear_addr( > unsigned int addr_size, > unsigned long *linear_addr) > { > - unsigned long addr = offset; > - uint32_t last_byte; > + unsigned long addr = offset, last_byte; > > if ( !(current->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE) ) > { > @@ -1941,6 +1940,9 @@ int hvm_virtual_to_linear_addr( > * Certain of them are not done in native real mode anyway. > */ > addr = (uint32_t)(addr + reg->base); > + last_byte = (uint32_t)addr + bytes - 1; > + if ( last_byte < addr ) > + return 0; > } > else if ( addr_size != 64 ) > { > @@ -1952,17 +1954,17 @@ int hvm_virtual_to_linear_addr( > { > case hvm_access_read: > if ( (reg->attr.fields.type & 0xa) == 0x8 ) > - goto gpf; /* execute-only code segment */ > + return 0; /* execute-only code segment */ > break; > case hvm_access_write: > if ( (reg->attr.fields.type & 0xa) != 0x2 ) > - goto gpf; /* not a writable data segment */ > + return 0; /* not a writable data segment */ > break; > default: > break; > } > > - last_byte = offset + bytes - 1; > + last_byte = (uint32_t)offset + bytes - 1; > > /* Is this a grows-down data segment? Special limit check if so. */ > if ( (reg->attr.fields.type & 0xc) == 0x4 ) > @@ -1973,10 +1975,10 @@ int hvm_virtual_to_linear_addr( > > /* Check first byte and last byte against respective bounds. */ > if ( (offset <= reg->limit) || (last_byte < offset) ) > - goto gpf; > + return 0; > } > else if ( (last_byte > reg->limit) || (last_byte < offset) ) > - goto gpf; /* last byte is beyond limit or wraps 0xFFFFFFFF */ > + return 0; /* last byte is beyond limit or wraps 0xFFFFFFFF */ > > /* > * Hardware truncates to 32 bits in compatibility mode. > @@ -1993,15 +1995,14 @@ int hvm_virtual_to_linear_addr( > if ( (seg == x86_seg_fs) || (seg == x86_seg_gs) ) > addr += reg->base; > > - if ( !is_canonical_address(addr) ) > - goto gpf; > + last_byte = addr + bytes - 1; > + if ( !is_canonical_address(addr) || last_byte < addr || > + !is_canonical_address(last_byte) ) > + return 0; > } > > *linear_addr = addr; > return 1; > - > - gpf: > - return 0; > } > > /* On non-NULL return, we leave this function holding an additional > > >