Xen devel - Jun 2012 - [PATCH] x86-64: don't allow non-canonical addresses to be set for any callback

Rather than deferring the detection of these to the point where they
get actually used (the fix for XSA-7, 25480:76eaf5966c05, causing a #GP
to be raised by IRET, which invokes the guest''s [fragile] fail-safe
callback), don''t even allow such to be set.

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

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -736,6 +736,14 @@ int arch_set_info_guest(
     {
         if ( !compat )
         {
+#ifdef __x86_64__
+            if ( !is_canonical_address(c.nat->user_regs.eip) ||
+                 !is_canonical_address(c.nat->event_callback_eip) ||
+                 !is_canonical_address(c.nat->syscall_callback_eip) ||
+                 !is_canonical_address(c.nat->failsafe_callback_eip) )
+                return -EINVAL;
+#endif
+
             fixup_guest_stack_selector(d, c.nat->user_regs.ss);
             fixup_guest_stack_selector(d, c.nat->kernel_ss);
             fixup_guest_code_selector(d, c.nat->user_regs.cs);
@@ -745,7 +753,11 @@ int arch_set_info_guest(
 #endif
 
             for ( i = 0; i < 256; i++ )
+            {
+                if ( !is_canonical_address(c.nat->trap_ctxt[i].address) )
+                    return -EINVAL;
                 fixup_guest_code_selector(d, c.nat->trap_ctxt[i].cs);
+            }
 
             /* LDT safety checks. */
             if ( ((c.nat->ldt_base & (PAGE_SIZE-1)) != 0) ||
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1033,6 +1033,9 @@ long arch_do_domctl(
 #ifdef __x86_64__
             if ( !is_hvm_domain(d) )
             {
+                if ( !is_canonical_address(evc->sysenter_callback_eip) ||
+                     !is_canonical_address(evc->syscall32_callback_eip) )
+                    goto ext_vcpucontext_out;
                 fixup_guest_code_selector(d, evc->sysenter_callback_cs);
                 v->arch.pv_vcpu.sysenter_callback_cs                        
evc->sysenter_callback_cs;
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3581,6 +3581,9 @@ long register_guest_nmi_callback(unsigne
     struct domain *d = v->domain;
     struct trap_info *t = &v->arch.pv_vcpu.trap_ctxt[TRAP_nmi];
 
+    if ( !is_canonical_address(address) )
+        return -EINVAL;
+
     t->vector  = TRAP_nmi;
     t->flags   = 0;
     t->cs      = (is_pv_32on64_domain(d) ?
@@ -3708,6 +3711,9 @@ long do_set_trap_table(XEN_GUEST_HANDLE(
         if ( cur.address == 0 )
             break;
 
+        if ( !is_canonical_address(cur.address) )
+            return -EINVAL;
+
         fixup_guest_code_selector(curr->domain, cur.cs);
 
         memcpy(&dst[cur.vector], &cur, sizeof(cur));





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

On 13/06/12 11:02, Jan Beulich wrote:
> Rather than deferring the detection of these to the point where they
> get actually used (the fix for XSA-7, 25480:76eaf5966c05, causing a #GP
> to be raised by IRET, which invokes the guest''s [fragile]
fail-safe
> callback), don''t even allow such to be set.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -736,6 +736,14 @@ int arch_set_info_guest(
>      {
>          if ( !compat )
>          {
> +#ifdef __x86_64__
> +            if ( !is_canonical_address(c.nat->user_regs.eip) ||
> +                 !is_canonical_address(c.nat->event_callback_eip) ||
> +                 !is_canonical_address(c.nat->syscall_callback_eip) ||
> +                 !is_canonical_address(c.nat->failsafe_callback_eip) )
> +                return -EINVAL;
> +#endif
> +
Would it be better to have

#ifndef __x86_64__
#  define is_canonical_address(a) 1
#endif

somewhere?

David

>>> On 13.06.12 at 12:51, David Vrabel <dvrabel@cantab.net>
wrote:
> On 13/06/12 11:02, Jan Beulich wrote:
>> Rather than deferring the detection of these to the point where they
>> get actually used (the fix for XSA-7, 25480:76eaf5966c05, causing a #GP
>> to be raised by IRET, which invokes the guest''s [fragile]
fail-safe
>> callback), don''t even allow such to be set.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -736,6 +736,14 @@ int arch_set_info_guest(
>>      {
>>          if ( !compat )
>>          {
>> +#ifdef __x86_64__
>> +            if ( !is_canonical_address(c.nat->user_regs.eip) ||
>> +                 !is_canonical_address(c.nat->event_callback_eip)
||
>> +                 !is_canonical_address(c.nat->syscall_callback_eip)
||
>> +                
!is_canonical_address(c.nat->failsafe_callback_eip) )
>> +                return -EINVAL;
>> +#endif
>> +
> 
> Would it be better to have
> 
> #ifndef __x86_64__
> #  define is_canonical_address(a) 1
> #endif
> 
> somewhere?
That one we have (otherwise other changes in this patch would
cause build failures), the problem here is that ->syscall_callback
doesn''t exist for the 32-bit interface. And rater than just putting
that on line into a conditional, I preferred to frame the whole
addition.

Jan

On 13/06/2012 11:02, "Jan Beulich" <JBeulich@suse.com> wrote:
> Rather than deferring the detection of these to the point where they
> get actually used (the fix for XSA-7, 25480:76eaf5966c05, causing a #GP
> to be raised by IRET, which invokes the guest''s [fragile]
fail-safe
> callback), don''t even allow such to be set.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -736,6 +736,14 @@ int arch_set_info_guest(
>      {
>          if ( !compat )
>          {
> +#ifdef __x86_64__
> +            if ( !is_canonical_address(c.nat->user_regs.eip) ||
> +                 !is_canonical_address(c.nat->event_callback_eip) ||
> +                 !is_canonical_address(c.nat->syscall_callback_eip) ||
> +                 !is_canonical_address(c.nat->failsafe_callback_eip) )
> +                return -EINVAL;
> +#endif
> +
>              fixup_guest_stack_selector(d, c.nat->user_regs.ss);
>              fixup_guest_stack_selector(d, c.nat->kernel_ss);
>              fixup_guest_code_selector(d, c.nat->user_regs.cs);
> @@ -745,7 +753,11 @@ int arch_set_info_guest(
>  #endif
>  
>              for ( i = 0; i < 256; i++ )
> +            {
> +                if ( !is_canonical_address(c.nat->trap_ctxt[i].address)
)
> +                    return -EINVAL;
>                  fixup_guest_code_selector(d, c.nat->trap_ctxt[i].cs);
> +            }
>  
>              /* LDT safety checks. */
>              if ( ((c.nat->ldt_base & (PAGE_SIZE-1)) != 0) ||
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1033,6 +1033,9 @@ long arch_do_domctl(
>  #ifdef __x86_64__
>              if ( !is_hvm_domain(d) )
>              {
> +                if ( !is_canonical_address(evc->sysenter_callback_eip)
||
> +                     !is_canonical_address(evc->syscall32_callback_eip)
)
> +                    goto ext_vcpucontext_out;
>                  fixup_guest_code_selector(d,
evc->sysenter_callback_cs);
>                  v->arch.pv_vcpu.sysenter_callback_cs      >         
evc->sysenter_callback_cs;
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -3581,6 +3581,9 @@ long register_guest_nmi_callback(unsigne
>      struct domain *d = v->domain;
>      struct trap_info *t = &v->arch.pv_vcpu.trap_ctxt[TRAP_nmi];
>  
> +    if ( !is_canonical_address(address) )
> +        return -EINVAL;
> +
>      t->vector  = TRAP_nmi;
>      t->flags   = 0;
>      t->cs      = (is_pv_32on64_domain(d) ?
> @@ -3708,6 +3711,9 @@ long do_set_trap_table(XEN_GUEST_HANDLE(
>          if ( cur.address == 0 )
>              break;
>  
> +        if ( !is_canonical_address(cur.address) )
> +            return -EINVAL;
> +
>          fixup_guest_code_selector(curr->domain, cur.cs);
>  
>          memcpy(&dst[cur.vector], &cur, sizeof(cur));
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel