[PATCH 0/3] x86: XSA-109/110 follow-up (to be considered for 4.5)

[PATCH 0/3] x86: XSA-109/110 follow-up (to be considered for 4.5)

[Jan Beulich]

1: tighten page table owner checking in do_mmu_update()
2: don't ignore foreigndom input on various MMUEXT ops
3: HVM: don't crash guest upon problems occurring in user mode

Reason to request considering this for 4.5: Tightened argument
checking (as done in the first two patches) reduces the chances
of them getting abused. Not unduly crashing the guest (as done
in the third one) may avoid future security issues of guest user
mode affecting the guest kernel.

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

[PATCH 1/3] x86: tighten page table owner checking in do_mmu_update()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

MMU_MACHPHYS_UPDATE, not manipulating page tables, shouldn't ignore
a bad page table domain being specified.

Also pt_owner can't be NULL when reaching the "out" label, so the
respective check can be dropped.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Tim Deegan <tim@xen.org>

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3618,6 +3618,11 @@ long do_mmu_update(
         break;
 
         case MMU_MACHPHYS_UPDATE:
+            if ( unlikely(d != pt_owner) )
+            {
+                rc = -EPERM;
+                break;
+            }
 
             mfn = req.ptr >> PAGE_SHIFT;
             gpfn = req.val;
@@ -3694,7 +3699,7 @@ long do_mmu_update(
     perfc_add(num_page_updates, i);
 
  out:
-    if ( pt_owner && (pt_owner != d) )
+    if ( pt_owner != d )
         rcu_unlock_domain(pt_owner);
 
     /* Add incremental work we have done to the @done output parameter. */




[-- Attachment #2: x86-mmuupd-check-foreign.patch --]
[-- Type: text/plain, Size: 1001 bytes --]

x86: tighten page table owner checking in do_mmu_update()

MMU_MACHPHYS_UPDATE, not manipulating page tables, shouldn't ignore
a bad page table domain being specified.

Also pt_owner can't be NULL when reaching the "out" label, so the
respective check can be dropped.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Tim Deegan <tim@xen.org>

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3618,6 +3618,11 @@ long do_mmu_update(
         break;
 
         case MMU_MACHPHYS_UPDATE:
+            if ( unlikely(d != pt_owner) )
+            {
+                rc = -EPERM;
+                break;
+            }
 
             mfn = req.ptr >> PAGE_SHIFT;
             gpfn = req.val;
@@ -3694,7 +3699,7 @@ long do_mmu_update(
     perfc_add(num_page_updates, i);
 
  out:
-    if ( pt_owner && (pt_owner != d) )
+    if ( pt_owner != d )
         rcu_unlock_domain(pt_owner);
 
     /* Add incremental work we have done to the @done output parameter. */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 1/3] x86: tighten page table owner checking in do_mmu_update()

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1370 bytes --]

On 20/11/14 10:11, Jan Beulich wrote:
> MMU_MACHPHYS_UPDATE, not manipulating page tables, shouldn't ignore
> a bad page table domain being specified.
>
> Also pt_owner can't be NULL when reaching the "out" label, so the
> respective check can be dropped.

Yes it can.

Failing

    if ( (pg_owner = get_pg_owner((uint16_t)foreigndom)) == NULL )
    {
        rc = -ESRCH;
        goto out;
    }

around line 3462 will cause pt_owner to be NULL at the out label.

~Andrew

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Tim Deegan <tim@xen.org>
>
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3618,6 +3618,11 @@ long do_mmu_update(
>          break;
>  
>          case MMU_MACHPHYS_UPDATE:
> +            if ( unlikely(d != pt_owner) )
> +            {
> +                rc = -EPERM;
> +                break;
> +            }
>  
>              mfn = req.ptr >> PAGE_SHIFT;
>              gpfn = req.val;
> @@ -3694,7 +3699,7 @@ long do_mmu_update(
>      perfc_add(num_page_updates, i);
>  
>   out:
> -    if ( pt_owner && (pt_owner != d) )
> +    if ( pt_owner != d )
>          rcu_unlock_domain(pt_owner);
>  
>      /* Add incremental work we have done to the @done output parameter. */
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 2401 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 1/3] x86: tighten page table owner checking in do_mmu_update()

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 639 bytes --]

On 20/11/14 10:29, Andrew Cooper wrote:
> On 20/11/14 10:11, Jan Beulich wrote:
>> MMU_MACHPHYS_UPDATE, not manipulating page tables, shouldn't ignore
>> a bad page table domain being specified.
>>
>> Also pt_owner can't be NULL when reaching the "out" label, so the
>> respective check can be dropped.
>
> Yes it can.
>
> Failing
>
>     if ( (pg_owner = get_pg_owner((uint16_t)foreigndom)) == NULL )
>     {
>         rc = -ESRCH;
>         goto out;
>     }
>
> around line 3462 will cause pt_owner to be NULL at the out label.
>
> ~Andrew

...And I should really double check my reply before I send.  Apologies
for the noise.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 1417 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH 2/3] x86: don't ignore foreigndom input on various MMUEXT ops

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 9388 bytes --]

Instead properly fail requests that shouldn't be issued on foreign
domains or - for MMUEXT_{CLEAR,COPY}_PAGE - extend the existing
operation to work that way.

In the course of doing this the need to always clear "okay" even when
wanting an error code other than -EINVAL became unwieldy, so the
respective logic is being adjusted at once, together with a little
other related cleanup.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3062,23 +3062,23 @@ long do_mmuext_op(
         }
 
         case MMUEXT_NEW_BASEPTR:
-            if ( paging_mode_translate(d) )
-                okay = 0;
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( unlikely(paging_mode_translate(d)) )
+                rc = -EINVAL;
             else
-            {
                 rc = new_guest_cr3(op.arg1.mfn);
-                okay = !rc;
-            }
             break;
 
         case MMUEXT_NEW_USER_BASEPTR: {
             unsigned long old_mfn;
 
-            if ( paging_mode_translate(current->domain) )
-            {
-                okay = 0;
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( unlikely(paging_mode_translate(d)) )
+                rc = -EINVAL;
+            if ( unlikely(rc) )
                 break;
-            }
 
             old_mfn = pagetable_get_pfn(curr->arch.guest_table_user);
             /*
@@ -3136,12 +3136,17 @@ long do_mmuext_op(
         }
         
         case MMUEXT_TLB_FLUSH_LOCAL:
-            flush_tlb_local();
+            if ( likely(d == pg_owner) )
+                flush_tlb_local();
+            else
+                rc = -EPERM;
             break;
     
         case MMUEXT_INVLPG_LOCAL:
-            if ( !paging_mode_enabled(d) 
-                 || paging_invlpg(curr, op.arg1.linear_addr) != 0 )
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( !paging_mode_enabled(d) ||
+                      paging_invlpg(curr, op.arg1.linear_addr) != 0 )
                 flush_tlb_one_local(op.arg1.linear_addr);
             break;
 
@@ -3150,13 +3155,16 @@ long do_mmuext_op(
         {
             cpumask_t pmask;
 
-            if ( unlikely(vcpumask_to_pcpumask(d,
-                            guest_handle_to_param(op.arg2.vcpumask, const_void),
-                            &pmask)) )
-            {
-                okay = 0;
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( unlikely(vcpumask_to_pcpumask(d,
+                                   guest_handle_to_param(op.arg2.vcpumask,
+                                                         const_void),
+                                   &pmask)) )
+                rc = -EINVAL;
+            if ( unlikely(rc) )
                 break;
-            }
+
             if ( op.cmd == MMUEXT_TLB_FLUSH_MULTI )
                 flush_tlb_mask(&pmask);
             else
@@ -3165,18 +3173,26 @@ long do_mmuext_op(
         }
 
         case MMUEXT_TLB_FLUSH_ALL:
-            flush_tlb_mask(d->domain_dirty_cpumask);
+            if ( likely(d == pg_owner) )
+                flush_tlb_mask(d->domain_dirty_cpumask);
+            else
+                rc = -EPERM;
             break;
     
         case MMUEXT_INVLPG_ALL:
-            flush_tlb_one_mask(d->domain_dirty_cpumask, op.arg1.linear_addr);
+            if ( likely(d == pg_owner) )
+                flush_tlb_one_mask(d->domain_dirty_cpumask, op.arg1.linear_addr);
+            else
+                rc = -EPERM;
             break;
 
         case MMUEXT_FLUSH_CACHE:
-            if ( unlikely(!cache_flush_permitted(d)) )
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( unlikely(!cache_flush_permitted(d)) )
             {
                 MEM_LOG("Non-physdev domain tried to FLUSH_CACHE.");
-                okay = 0;
+                rc = -EACCES;
             }
             else
             {
@@ -3185,8 +3201,8 @@ long do_mmuext_op(
             break;
 
         case MMUEXT_FLUSH_CACHE_GLOBAL:
-            if ( unlikely(foreigndom != DOMID_SELF) )
-                okay = 0;
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
             else if ( likely(cache_flush_permitted(d)) )
             {
                 unsigned int cpu;
@@ -3211,7 +3227,9 @@ long do_mmuext_op(
             unsigned long ptr  = op.arg1.linear_addr;
             unsigned long ents = op.arg2.nr_ents;
 
-            if ( paging_mode_external(d) )
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( paging_mode_external(d) )
             {
                 MEM_LOG("ignoring SET_LDT hypercall from external domain");
                 okay = 0;
@@ -3237,7 +3255,7 @@ long do_mmuext_op(
         case MMUEXT_CLEAR_PAGE: {
             struct page_info *page;
 
-            page = get_page_from_gfn(d, op.arg1.mfn, NULL, P2M_ALLOC);
+            page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC);
             if ( !page || !get_page_type(page, PGT_writable_page) )
             {
                 if ( page )
@@ -3248,7 +3266,7 @@ long do_mmuext_op(
             }
 
             /* A page is dirtied when it's being cleared. */
-            paging_mark_dirty(d, page_to_mfn(page));
+            paging_mark_dirty(pg_owner, page_to_mfn(page));
 
             clear_domain_page(page_to_mfn(page));
 
@@ -3260,7 +3278,8 @@ long do_mmuext_op(
         {
             struct page_info *src_page, *dst_page;
 
-            src_page = get_page_from_gfn(d, op.arg2.src_mfn, NULL, P2M_ALLOC);
+            src_page = get_page_from_gfn(pg_owner, op.arg2.src_mfn, NULL,
+                                         P2M_ALLOC);
             if ( unlikely(!src_page) )
             {
                 okay = 0;
@@ -3268,7 +3287,8 @@ long do_mmuext_op(
                 break;
             }
 
-            dst_page = get_page_from_gfn(d, op.arg1.mfn, NULL, P2M_ALLOC);
+            dst_page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL,
+                                         P2M_ALLOC);
             okay = (dst_page && get_page_type(dst_page, PGT_writable_page));
             if ( unlikely(!okay) )
             {
@@ -3280,7 +3300,7 @@ long do_mmuext_op(
             }
 
             /* A page is dirtied when it's being copied to. */
-            paging_mark_dirty(d, page_to_mfn(dst_page));
+            paging_mark_dirty(pg_owner, page_to_mfn(dst_page));
 
             copy_domain_page(page_to_mfn(dst_page), page_to_mfn(src_page));
 
@@ -3291,68 +3311,56 @@ long do_mmuext_op(
 
         case MMUEXT_MARK_SUPER:
         {
-            unsigned long mfn;
-            struct spage_info *spage;
+            unsigned long mfn = op.arg1.mfn;
 
-            mfn = op.arg1.mfn;
-            if ( mfn & (L1_PAGETABLE_ENTRIES-1) )
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( mfn & (L1_PAGETABLE_ENTRIES-1) )
             {
                 MEM_LOG("Unaligned superpage reference mfn %lx", mfn);
                 okay = 0;
-                break;
             }
-
-            if ( !opt_allow_superpage )
+            else if ( !opt_allow_superpage )
             {
                 MEM_LOG("Superpages disallowed");
-                okay = 0;
                 rc = -ENOSYS;
-                break;
             }
-
-            spage = mfn_to_spage(mfn);
-            okay = (mark_superpage(spage, d) >= 0);
+            else
+                rc = mark_superpage(mfn_to_spage(mfn), d);
             break;
         }
 
         case MMUEXT_UNMARK_SUPER:
         {
-            unsigned long mfn;
-            struct spage_info *spage;
+            unsigned long mfn = op.arg1.mfn;
 
-            mfn = op.arg1.mfn;
-            if ( mfn & (L1_PAGETABLE_ENTRIES-1) )
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( mfn & (L1_PAGETABLE_ENTRIES-1) )
             {
                 MEM_LOG("Unaligned superpage reference mfn %lx", mfn);
                 okay = 0;
-                break;
             }
-
-            if ( !opt_allow_superpage )
+            else if ( !opt_allow_superpage )
             {
                 MEM_LOG("Superpages disallowed");
-                okay = 0;
                 rc = -ENOSYS;
-                break;
             }
-
-            spage = mfn_to_spage(mfn);
-            okay = (unmark_superpage(spage) >= 0);
+            else
+                rc = unmark_superpage(mfn_to_spage(mfn));
             break;
         }
 
         default:
             MEM_LOG("Invalid extended pt command %#x", op.cmd);
             rc = -ENOSYS;
-            okay = 0;
             break;
         }
 
-        if ( unlikely(!okay) )
-        {
-            rc = rc ? rc : -EINVAL;
+        if ( unlikely(!okay) && !rc )
+            rc = -EINVAL;
+        if ( unlikely(rc) )
             break;
-        }
 
         guest_handle_add_offset(uops, 1);
     }



[-- Attachment #2: x86-mmuext-check-foreign.patch --]
[-- Type: text/plain, Size: 9444 bytes --]

x86: don't ignore foreigndom input on various MMUEXT ops

Instead properly fail requests that shouldn't be issued on foreign
domains or - for MMUEXT_{CLEAR,COPY}_PAGE - extend the existing
operation to work that way.

In the course of doing this the need to always clear "okay" even when
wanting an error code other than -EINVAL became unwieldy, so the
respective logic is being adjusted at once, together with a little
other related cleanup.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3062,23 +3062,23 @@ long do_mmuext_op(
         }
 
         case MMUEXT_NEW_BASEPTR:
-            if ( paging_mode_translate(d) )
-                okay = 0;
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( unlikely(paging_mode_translate(d)) )
+                rc = -EINVAL;
             else
-            {
                 rc = new_guest_cr3(op.arg1.mfn);
-                okay = !rc;
-            }
             break;
 
         case MMUEXT_NEW_USER_BASEPTR: {
             unsigned long old_mfn;
 
-            if ( paging_mode_translate(current->domain) )
-            {
-                okay = 0;
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( unlikely(paging_mode_translate(d)) )
+                rc = -EINVAL;
+            if ( unlikely(rc) )
                 break;
-            }
 
             old_mfn = pagetable_get_pfn(curr->arch.guest_table_user);
             /*
@@ -3136,12 +3136,17 @@ long do_mmuext_op(
         }
         
         case MMUEXT_TLB_FLUSH_LOCAL:
-            flush_tlb_local();
+            if ( likely(d == pg_owner) )
+                flush_tlb_local();
+            else
+                rc = -EPERM;
             break;
     
         case MMUEXT_INVLPG_LOCAL:
-            if ( !paging_mode_enabled(d) 
-                 || paging_invlpg(curr, op.arg1.linear_addr) != 0 )
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( !paging_mode_enabled(d) ||
+                      paging_invlpg(curr, op.arg1.linear_addr) != 0 )
                 flush_tlb_one_local(op.arg1.linear_addr);
             break;
 
@@ -3150,13 +3155,16 @@ long do_mmuext_op(
         {
             cpumask_t pmask;
 
-            if ( unlikely(vcpumask_to_pcpumask(d,
-                            guest_handle_to_param(op.arg2.vcpumask, const_void),
-                            &pmask)) )
-            {
-                okay = 0;
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( unlikely(vcpumask_to_pcpumask(d,
+                                   guest_handle_to_param(op.arg2.vcpumask,
+                                                         const_void),
+                                   &pmask)) )
+                rc = -EINVAL;
+            if ( unlikely(rc) )
                 break;
-            }
+
             if ( op.cmd == MMUEXT_TLB_FLUSH_MULTI )
                 flush_tlb_mask(&pmask);
             else
@@ -3165,18 +3173,26 @@ long do_mmuext_op(
         }
 
         case MMUEXT_TLB_FLUSH_ALL:
-            flush_tlb_mask(d->domain_dirty_cpumask);
+            if ( likely(d == pg_owner) )
+                flush_tlb_mask(d->domain_dirty_cpumask);
+            else
+                rc = -EPERM;
             break;
     
         case MMUEXT_INVLPG_ALL:
-            flush_tlb_one_mask(d->domain_dirty_cpumask, op.arg1.linear_addr);
+            if ( likely(d == pg_owner) )
+                flush_tlb_one_mask(d->domain_dirty_cpumask, op.arg1.linear_addr);
+            else
+                rc = -EPERM;
             break;
 
         case MMUEXT_FLUSH_CACHE:
-            if ( unlikely(!cache_flush_permitted(d)) )
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( unlikely(!cache_flush_permitted(d)) )
             {
                 MEM_LOG("Non-physdev domain tried to FLUSH_CACHE.");
-                okay = 0;
+                rc = -EACCES;
             }
             else
             {
@@ -3185,8 +3201,8 @@ long do_mmuext_op(
             break;
 
         case MMUEXT_FLUSH_CACHE_GLOBAL:
-            if ( unlikely(foreigndom != DOMID_SELF) )
-                okay = 0;
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
             else if ( likely(cache_flush_permitted(d)) )
             {
                 unsigned int cpu;
@@ -3211,7 +3227,9 @@ long do_mmuext_op(
             unsigned long ptr  = op.arg1.linear_addr;
             unsigned long ents = op.arg2.nr_ents;
 
-            if ( paging_mode_external(d) )
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( paging_mode_external(d) )
             {
                 MEM_LOG("ignoring SET_LDT hypercall from external domain");
                 okay = 0;
@@ -3237,7 +3255,7 @@ long do_mmuext_op(
         case MMUEXT_CLEAR_PAGE: {
             struct page_info *page;
 
-            page = get_page_from_gfn(d, op.arg1.mfn, NULL, P2M_ALLOC);
+            page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC);
             if ( !page || !get_page_type(page, PGT_writable_page) )
             {
                 if ( page )
@@ -3248,7 +3266,7 @@ long do_mmuext_op(
             }
 
             /* A page is dirtied when it's being cleared. */
-            paging_mark_dirty(d, page_to_mfn(page));
+            paging_mark_dirty(pg_owner, page_to_mfn(page));
 
             clear_domain_page(page_to_mfn(page));
 
@@ -3260,7 +3278,8 @@ long do_mmuext_op(
         {
             struct page_info *src_page, *dst_page;
 
-            src_page = get_page_from_gfn(d, op.arg2.src_mfn, NULL, P2M_ALLOC);
+            src_page = get_page_from_gfn(pg_owner, op.arg2.src_mfn, NULL,
+                                         P2M_ALLOC);
             if ( unlikely(!src_page) )
             {
                 okay = 0;
@@ -3268,7 +3287,8 @@ long do_mmuext_op(
                 break;
             }
 
-            dst_page = get_page_from_gfn(d, op.arg1.mfn, NULL, P2M_ALLOC);
+            dst_page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL,
+                                         P2M_ALLOC);
             okay = (dst_page && get_page_type(dst_page, PGT_writable_page));
             if ( unlikely(!okay) )
             {
@@ -3280,7 +3300,7 @@ long do_mmuext_op(
             }
 
             /* A page is dirtied when it's being copied to. */
-            paging_mark_dirty(d, page_to_mfn(dst_page));
+            paging_mark_dirty(pg_owner, page_to_mfn(dst_page));
 
             copy_domain_page(page_to_mfn(dst_page), page_to_mfn(src_page));
 
@@ -3291,68 +3311,56 @@ long do_mmuext_op(
 
         case MMUEXT_MARK_SUPER:
         {
-            unsigned long mfn;
-            struct spage_info *spage;
+            unsigned long mfn = op.arg1.mfn;
 
-            mfn = op.arg1.mfn;
-            if ( mfn & (L1_PAGETABLE_ENTRIES-1) )
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( mfn & (L1_PAGETABLE_ENTRIES-1) )
             {
                 MEM_LOG("Unaligned superpage reference mfn %lx", mfn);
                 okay = 0;
-                break;
             }
-
-            if ( !opt_allow_superpage )
+            else if ( !opt_allow_superpage )
             {
                 MEM_LOG("Superpages disallowed");
-                okay = 0;
                 rc = -ENOSYS;
-                break;
             }
-
-            spage = mfn_to_spage(mfn);
-            okay = (mark_superpage(spage, d) >= 0);
+            else
+                rc = mark_superpage(mfn_to_spage(mfn), d);
             break;
         }
 
         case MMUEXT_UNMARK_SUPER:
         {
-            unsigned long mfn;
-            struct spage_info *spage;
+            unsigned long mfn = op.arg1.mfn;
 
-            mfn = op.arg1.mfn;
-            if ( mfn & (L1_PAGETABLE_ENTRIES-1) )
+            if ( unlikely(d != pg_owner) )
+                rc = -EPERM;
+            else if ( mfn & (L1_PAGETABLE_ENTRIES-1) )
             {
                 MEM_LOG("Unaligned superpage reference mfn %lx", mfn);
                 okay = 0;
-                break;
             }
-
-            if ( !opt_allow_superpage )
+            else if ( !opt_allow_superpage )
             {
                 MEM_LOG("Superpages disallowed");
-                okay = 0;
                 rc = -ENOSYS;
-                break;
             }
-
-            spage = mfn_to_spage(mfn);
-            okay = (unmark_superpage(spage) >= 0);
+            else
+                rc = unmark_superpage(mfn_to_spage(mfn));
             break;
         }
 
         default:
             MEM_LOG("Invalid extended pt command %#x", op.cmd);
             rc = -ENOSYS;
-            okay = 0;
             break;
         }
 
-        if ( unlikely(!okay) )
-        {
-            rc = rc ? rc : -EINVAL;
+        if ( unlikely(!okay) && !rc )
+            rc = -EINVAL;
+        if ( unlikely(rc) )
             break;
-        }
 
         guest_handle_add_offset(uops, 1);
     }

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/3] x86: don't ignore foreigndom input on various MMUEXT ops

[Tim Deegan]

At 11:12 +0100 on 20 Nov (1416478351), Jan Beulich wrote:
> Instead properly fail requests that shouldn't be issued on foreign
> domains or - for MMUEXT_{CLEAR,COPY}_PAGE - extend the existing
> operation to work that way.
> 
> In the course of doing this the need to always clear "okay" even when
> wanting an error code other than -EINVAL became unwieldy, so the
> respective logic is being adjusted at once, together with a little
> other related cleanup.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Tim Deegan <tim@xen.org>

though I would prefer this as two patches, one to remove 'okay'
altogether in favour of appropriate rc settings, and then a simpler
version of this.

Tim.

Re: [PATCH 2/3] x86: don't ignore foreigndom input on various MMUEXT ops

[George Dunlap]

On Thu, Nov 20, 2014 at 10:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
> Instead properly fail requests that shouldn't be issued on foreign
> domains or - for MMUEXT_{CLEAR,COPY}_PAGE - extend the existing
> operation to work that way.

I take it this is for 4.6?

I've looked through it and everything looks OK.

But I agree with Tim, that having so many different changes all at the
same time makes the patch hard to review.

In particular, I'd rather start with a patch to get rid of "okay"
entirely; then make MMUEXT_{CLEAR,COPY}_PAGE use foreingndom instead
of current; then have a patch which returns -EPERM for the other ones;
then a patch to get rid of spage in MMUEXT_[UN]MARK_SUPER.

Regarding MMUEXT_{CLEAR,COPY}_PAGE: This is effectively changing the
interface.  Are we sure there are no callers which just expect them to
work on current, and don't set foreigndom properly?

 -George

Re: [PATCH 2/3] x86: don't ignore foreigndom input on various MMUEXT ops

[Jan Beulich]

>>> On 24.11.14 at 13:43, <dunlapg@umich.edu> wrote:
> On Thu, Nov 20, 2014 at 10:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> Instead properly fail requests that shouldn't be issued on foreign
>> domains or - for MMUEXT_{CLEAR,COPY}_PAGE - extend the existing
>> operation to work that way.
> 
> I take it this is for 4.6?

Not really, as said in the cover letter.

> I've looked through it and everything looks OK.
> 
> But I agree with Tim, that having so many different changes all at the
> same time makes the patch hard to review.
> 
> In particular, I'd rather start with a patch to get rid of "okay"
> entirely; then make MMUEXT_{CLEAR,COPY}_PAGE use foreingndom instead
> of current; then have a patch which returns -EPERM for the other ones;
> then a patch to get rid of spage in MMUEXT_[UN]MARK_SUPER.

Yes, that's how I would have done it following Tim's comments if
there wasn't the desire for backporting - that'll become quite a bit
more involved if I do the cleanup patch removing "okay" first. And
doing the -EPERM one first would mean that the backport needs to
carefully add properly setting "okay". Splitting the clear/copy page
change from the -EPERM one doesn't make much sense to me at
all considering the title (and hence purpose) of the patch.

> Regarding MMUEXT_{CLEAR,COPY}_PAGE: This is effectively changing the
> interface.  Are we sure there are no callers which just expect them to
> work on current, and don't set foreigndom properly?

As much or as little as "all of the sudden" returning -EPERM in such
a case for other sub-ops. They never were meant to be used that
way, and the original omission of those checks shouldn't preclude us
from adding them.

Jan

[PATCH 3/3] x86/HVM: don't crash guest upon problems occurring in user mode

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4299 bytes --]

This extends commit 5283b310 ("x86/HVM: only kill guest when unknown VM
exit occurred in guest kernel mode") to further cases, including the
failed VM entry one that XSA-110 was needed to be issued for.

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

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -90,6 +90,15 @@ static bool_t amd_erratum383_found __rea
 static uint64_t osvw_length, osvw_status;
 static DEFINE_SPINLOCK(osvw_lock);
 
+/* Only crash the guest if the problem originates in kernel mode. */
+static void svm_crash_or_gp(struct vcpu *v)
+{
+    if ( vmcb_get_cpl(v->arch.hvm_svm.vmcb) )
+        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
+    else
+        domain_crash(v->domain);
+}
+
 void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len)
 {
     struct vcpu *curr = current;
@@ -100,7 +109,7 @@ void __update_guest_eip(struct cpu_user_
     if ( unlikely(inst_len > 15) )
     {
         gdprintk(XENLOG_ERR, "Bad instruction length %u\n", inst_len);
-        domain_crash(curr->domain);
+        svm_crash_or_gp(curr);
         return;
     }
 
@@ -1505,7 +1514,7 @@ static void svm_do_nested_pgfault(struct
     gdprintk(XENLOG_ERR,
          "SVM violation gpa %#"PRIpaddr", mfn %#lx, type %i\n",
          gpa, mfn_x(mfn), p2mt);
-    domain_crash(v->domain);
+    svm_crash_or_gp(v);
 }
 
 static void svm_fpu_dirty_intercept(void)
@@ -2647,7 +2656,7 @@ void svm_vmexit_handler(struct cpu_user_
             printk(XENLOG_G_ERR
                    "%pv: Error %d handling NPF (gpa=%08lx ec=%04lx)\n",
                    v, rc, vmcb->exitinfo2, vmcb->exitinfo1);
-            domain_crash(v->domain);
+            svm_crash_or_gp(v);
         }
         v->arch.hvm_svm.cached_insn_len = 0;
         break;
@@ -2680,11 +2689,7 @@ void svm_vmexit_handler(struct cpu_user_
                  "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
                  exit_reason, 
                  (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
-        if ( vmcb_get_cpl(vmcb) )
-            hvm_inject_hw_exception(TRAP_invalid_op,
-                                    HVM_DELIVER_NO_ERROR_CODE);
-        else
-            domain_crash(v->domain);
+        svm_crash_or_gp(v);
         break;
     }
 
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -134,6 +134,18 @@ static void vmx_vcpu_destroy(struct vcpu
     passive_domain_destroy(v);
 }
 
+/* Only crash the guest if the problem originates in kernel mode. */
+static void vmx_crash_or_gp(struct vcpu *v)
+{
+    struct segment_register ss;
+
+    vmx_get_segment_register(v, x86_seg_ss, &ss);
+    if ( ss.attr.fields.dpl )
+        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
+    else
+        domain_crash(v->domain);
+}
+
 static DEFINE_PER_CPU(struct vmx_msr_state, host_msr_state);
 
 static const u32 msr_index[] =
@@ -2474,7 +2486,7 @@ static void ept_handle_violation(unsigne
     if ( qualification & EPT_GLA_VALID )
         gdprintk(XENLOG_ERR, " --- GLA %#lx\n", gla);
 
-    domain_crash(d);
+    vmx_crash_or_gp(current);
 }
 
 static void vmx_failed_vmentry(unsigned int exit_reason,
@@ -2508,7 +2520,7 @@ static void vmx_failed_vmentry(unsigned 
     vmcs_dump_vcpu(curr);
     printk("**************************************\n");
 
-    domain_crash(curr->domain);
+    vmx_crash_or_gp(curr);
 }
 
 void vmx_enter_realmode(struct cpu_user_regs *regs)
@@ -3161,19 +3173,8 @@ void vmx_vmexit_handler(struct cpu_user_
     /* fall through */
     default:
     exit_and_crash:
-        {
-            struct segment_register ss;
-
-            gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n",
-                     exit_reason);
-
-            vmx_get_segment_register(v, x86_seg_ss, &ss);
-            if ( ss.attr.fields.dpl )
-                hvm_inject_hw_exception(TRAP_invalid_op,
-                                        HVM_DELIVER_NO_ERROR_CODE);
-            else
-                domain_crash(v->domain);
-        }
+        gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n", exit_reason);
+        vmx_crash_or_gp(v);
         break;
     }
 



[-- Attachment #2: x86-HVM-dont-crash-guest-in-user-mode.patch --]
[-- Type: text/plain, Size: 4362 bytes --]

x86/HVM: don't crash guest upon problems occurring in user mode

This extends commit 5283b310 ("x86/HVM: only kill guest when unknown VM
exit occurred in guest kernel mode") to further cases, including the
failed VM entry one that XSA-110 was needed to be issued for.

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

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -90,6 +90,15 @@ static bool_t amd_erratum383_found __rea
 static uint64_t osvw_length, osvw_status;
 static DEFINE_SPINLOCK(osvw_lock);
 
+/* Only crash the guest if the problem originates in kernel mode. */
+static void svm_crash_or_gp(struct vcpu *v)
+{
+    if ( vmcb_get_cpl(v->arch.hvm_svm.vmcb) )
+        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
+    else
+        domain_crash(v->domain);
+}
+
 void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len)
 {
     struct vcpu *curr = current;
@@ -100,7 +109,7 @@ void __update_guest_eip(struct cpu_user_
     if ( unlikely(inst_len > 15) )
     {
         gdprintk(XENLOG_ERR, "Bad instruction length %u\n", inst_len);
-        domain_crash(curr->domain);
+        svm_crash_or_gp(curr);
         return;
     }
 
@@ -1505,7 +1514,7 @@ static void svm_do_nested_pgfault(struct
     gdprintk(XENLOG_ERR,
          "SVM violation gpa %#"PRIpaddr", mfn %#lx, type %i\n",
          gpa, mfn_x(mfn), p2mt);
-    domain_crash(v->domain);
+    svm_crash_or_gp(v);
 }
 
 static void svm_fpu_dirty_intercept(void)
@@ -2647,7 +2656,7 @@ void svm_vmexit_handler(struct cpu_user_
             printk(XENLOG_G_ERR
                    "%pv: Error %d handling NPF (gpa=%08lx ec=%04lx)\n",
                    v, rc, vmcb->exitinfo2, vmcb->exitinfo1);
-            domain_crash(v->domain);
+            svm_crash_or_gp(v);
         }
         v->arch.hvm_svm.cached_insn_len = 0;
         break;
@@ -2680,11 +2689,7 @@ void svm_vmexit_handler(struct cpu_user_
                  "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
                  exit_reason, 
                  (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
-        if ( vmcb_get_cpl(vmcb) )
-            hvm_inject_hw_exception(TRAP_invalid_op,
-                                    HVM_DELIVER_NO_ERROR_CODE);
-        else
-            domain_crash(v->domain);
+        svm_crash_or_gp(v);
         break;
     }
 
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -134,6 +134,18 @@ static void vmx_vcpu_destroy(struct vcpu
     passive_domain_destroy(v);
 }
 
+/* Only crash the guest if the problem originates in kernel mode. */
+static void vmx_crash_or_gp(struct vcpu *v)
+{
+    struct segment_register ss;
+
+    vmx_get_segment_register(v, x86_seg_ss, &ss);
+    if ( ss.attr.fields.dpl )
+        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
+    else
+        domain_crash(v->domain);
+}
+
 static DEFINE_PER_CPU(struct vmx_msr_state, host_msr_state);
 
 static const u32 msr_index[] =
@@ -2474,7 +2486,7 @@ static void ept_handle_violation(unsigne
     if ( qualification & EPT_GLA_VALID )
         gdprintk(XENLOG_ERR, " --- GLA %#lx\n", gla);
 
-    domain_crash(d);
+    vmx_crash_or_gp(current);
 }
 
 static void vmx_failed_vmentry(unsigned int exit_reason,
@@ -2508,7 +2520,7 @@ static void vmx_failed_vmentry(unsigned 
     vmcs_dump_vcpu(curr);
     printk("**************************************\n");
 
-    domain_crash(curr->domain);
+    vmx_crash_or_gp(curr);
 }
 
 void vmx_enter_realmode(struct cpu_user_regs *regs)
@@ -3161,19 +3173,8 @@ void vmx_vmexit_handler(struct cpu_user_
     /* fall through */
     default:
     exit_and_crash:
-        {
-            struct segment_register ss;
-
-            gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n",
-                     exit_reason);
-
-            vmx_get_segment_register(v, x86_seg_ss, &ss);
-            if ( ss.attr.fields.dpl )
-                hvm_inject_hw_exception(TRAP_invalid_op,
-                                        HVM_DELIVER_NO_ERROR_CODE);
-            else
-                domain_crash(v->domain);
-        }
+        gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n", exit_reason);
+        vmx_crash_or_gp(v);
         break;
     }
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 3/3] x86/HVM: don't crash guest upon problems occurring in user mode

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 4952 bytes --]

On 20/11/14 10:13, Jan Beulich wrote:
> This extends commit 5283b310 ("x86/HVM: only kill guest when unknown VM
> exit occurred in guest kernel mode") to further cases, including the
> failed VM entry one that XSA-110 was needed to be issued for.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -90,6 +90,15 @@ static bool_t amd_erratum383_found __rea
>  static uint64_t osvw_length, osvw_status;
>  static DEFINE_SPINLOCK(osvw_lock);
>  
> +/* Only crash the guest if the problem originates in kernel mode. */
> +static void svm_crash_or_gp(struct vcpu *v)
> +{
> +    if ( vmcb_get_cpl(v->arch.hvm_svm.vmcb) )
> +        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);

This (and its VMX counterpart) should either deliver a #GP fault, or
have its name changed to not imply a #GP fault.

How about "crash_or_fault()" as an alternative which is slightly less
specific?

~Andrew

> +    else
> +        domain_crash(v->domain);
> +}
> +
>  void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len)
>  {
>      struct vcpu *curr = current;
> @@ -100,7 +109,7 @@ void __update_guest_eip(struct cpu_user_
>      if ( unlikely(inst_len > 15) )
>      {
>          gdprintk(XENLOG_ERR, "Bad instruction length %u\n", inst_len);
> -        domain_crash(curr->domain);
> +        svm_crash_or_gp(curr);
>          return;
>      }
>  
> @@ -1505,7 +1514,7 @@ static void svm_do_nested_pgfault(struct
>      gdprintk(XENLOG_ERR,
>           "SVM violation gpa %#"PRIpaddr", mfn %#lx, type %i\n",
>           gpa, mfn_x(mfn), p2mt);
> -    domain_crash(v->domain);
> +    svm_crash_or_gp(v);
>  }
>  
>  static void svm_fpu_dirty_intercept(void)
> @@ -2647,7 +2656,7 @@ void svm_vmexit_handler(struct cpu_user_
>              printk(XENLOG_G_ERR
>                     "%pv: Error %d handling NPF (gpa=%08lx ec=%04lx)\n",
>                     v, rc, vmcb->exitinfo2, vmcb->exitinfo1);
> -            domain_crash(v->domain);
> +            svm_crash_or_gp(v);
>          }
>          v->arch.hvm_svm.cached_insn_len = 0;
>          break;
> @@ -2680,11 +2689,7 @@ void svm_vmexit_handler(struct cpu_user_
>                   "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
>                   exit_reason, 
>                   (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
> -        if ( vmcb_get_cpl(vmcb) )
> -            hvm_inject_hw_exception(TRAP_invalid_op,
> -                                    HVM_DELIVER_NO_ERROR_CODE);
> -        else
> -            domain_crash(v->domain);
> +        svm_crash_or_gp(v);
>          break;
>      }
>  
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -134,6 +134,18 @@ static void vmx_vcpu_destroy(struct vcpu
>      passive_domain_destroy(v);
>  }
>  
> +/* Only crash the guest if the problem originates in kernel mode. */
> +static void vmx_crash_or_gp(struct vcpu *v)
> +{
> +    struct segment_register ss;
> +
> +    vmx_get_segment_register(v, x86_seg_ss, &ss);
> +    if ( ss.attr.fields.dpl )
> +        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
> +    else
> +        domain_crash(v->domain);
> +}
> +
>  static DEFINE_PER_CPU(struct vmx_msr_state, host_msr_state);
>  
>  static const u32 msr_index[] =
> @@ -2474,7 +2486,7 @@ static void ept_handle_violation(unsigne
>      if ( qualification & EPT_GLA_VALID )
>          gdprintk(XENLOG_ERR, " --- GLA %#lx\n", gla);
>  
> -    domain_crash(d);
> +    vmx_crash_or_gp(current);
>  }
>  
>  static void vmx_failed_vmentry(unsigned int exit_reason,
> @@ -2508,7 +2520,7 @@ static void vmx_failed_vmentry(unsigned 
>      vmcs_dump_vcpu(curr);
>      printk("**************************************\n");
>  
> -    domain_crash(curr->domain);
> +    vmx_crash_or_gp(curr);
>  }
>  
>  void vmx_enter_realmode(struct cpu_user_regs *regs)
> @@ -3161,19 +3173,8 @@ void vmx_vmexit_handler(struct cpu_user_
>      /* fall through */
>      default:
>      exit_and_crash:
> -        {
> -            struct segment_register ss;
> -
> -            gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n",
> -                     exit_reason);
> -
> -            vmx_get_segment_register(v, x86_seg_ss, &ss);
> -            if ( ss.attr.fields.dpl )
> -                hvm_inject_hw_exception(TRAP_invalid_op,
> -                                        HVM_DELIVER_NO_ERROR_CODE);
> -            else
> -                domain_crash(v->domain);
> -        }
> +        gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n", exit_reason);
> +        vmx_crash_or_gp(v);
>          break;
>      }
>  
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 5497 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 3/3] x86/HVM: don't crash guest upon problems occurring in user mode

[Jan Beulich]

>>> On 20.11.14 at 12:06, <andrew.cooper3@citrix.com> wrote:
> On 20/11/14 10:13, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -90,6 +90,15 @@ static bool_t amd_erratum383_found __rea
>>  static uint64_t osvw_length, osvw_status;
>>  static DEFINE_SPINLOCK(osvw_lock);
>>  
>> +/* Only crash the guest if the problem originates in kernel mode. */
>> +static void svm_crash_or_gp(struct vcpu *v)
>> +{
>> +    if ( vmcb_get_cpl(v->arch.hvm_svm.vmcb) )
>> +        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
> 
> This (and its VMX counterpart) should either deliver a #GP fault, or
> have its name changed to not imply a #GP fault.

Indeed - not sure how I managed to get this mixed up.

Jan

Re: [PATCH 3/3] x86/HVM: don't crash guest upon problems occurring in user mode

[Tim Deegan]

At 11:13 +0100 on 20 Nov (1416478386), Jan Beulich wrote:
> This extends commit 5283b310 ("x86/HVM: only kill guest when unknown VM
> exit occurred in guest kernel mode") to further cases, including the
> failed VM entry one that XSA-110 was needed to be issued for.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This seems like a good idea in general, but I'm not sure it's
appropriate for _all_ of these.  Unhandled exit types and 
overlong instruction decode seem obviously good. 

hvm_hap_nested_page_fault() returns 0: seems only to happen for pvh
guests that write to read-only memory (?).  That seems like a
different class of failure.  I don't think our response should be
different based on the privilege level here, although domain_crash()
does seem harsh.  (I presume this is to avoid emulating an instruction
in PVH mode?)  If we're changing this, I think it should be to #GP
rather than #UD.

p2m_pt_handle_deferred_changes() returns < 0: AFAICS this is basically
ENOMEM when trying to update p2m tables.  It's so unlikely to be
caused by userspace activity that disguising it with #UD is probably
just unhelpful.  It turns a clean failure into an undebuggable
intermittent glitch.

bad vm entry: Here we're basically looking at a Xen bug that we're
just trying to contain the damage on.  I guess maybe if the guest user
can trigger it it's nice to give the kernel a chance.  And at least it
comes with a loud console message, so I'm OK with it. 

Cheers,

Tim.

Re: [PATCH 3/3] x86/HVM: don't crash guest upon problems occurring in user mode

[Jan Beulich]

>>> On 20.11.14 at 12:34, <tim@xen.org> wrote:
> At 11:13 +0100 on 20 Nov (1416478386), Jan Beulich wrote:
>> This extends commit 5283b310 ("x86/HVM: only kill guest when unknown VM
>> exit occurred in guest kernel mode") to further cases, including the
>> failed VM entry one that XSA-110 was needed to be issued for.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> This seems like a good idea in general, but I'm not sure it's
> appropriate for _all_ of these.  Unhandled exit types and 
> overlong instruction decode seem obviously good. 
> 
> hvm_hap_nested_page_fault() returns 0: seems only to happen for pvh
> guests that write to read-only memory (?).  That seems like a
> different class of failure.  I don't think our response should be
> different based on the privilege level here, although domain_crash()
> does seem harsh.  (I presume this is to avoid emulating an instruction
> in PVH mode?)  If we're changing this, I think it should be to #GP
> rather than #UD.

I dropped those for now. We should re-evaluate this once we
have #VE support on VMX (i.e. we may want to inject that one
there instead).

> p2m_pt_handle_deferred_changes() returns < 0: AFAICS this is basically
> ENOMEM when trying to update p2m tables.  It's so unlikely to be
> caused by userspace activity that disguising it with #UD is probably
> just unhelpful.  It turns a clean failure into an undebuggable
> intermittent glitch.

Dropped too.

Jan

[PATCH v2 3/3] x86/HVM: don't crash guest upon problems occurring in user mode

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3519 bytes --]

This extends commit 5283b310 ("x86/HVM: only kill guest when unknown VM
exit occurred in guest kernel mode") to a few more cases, including the
failed VM entry one that XSA-110 was needed to be issued for.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: - s/crash_or_gp/crash_or_fault/
    - drop changes to svm_do_nested_pgfault(), svm_vmexit_handler()'s
      VMEXIT_NPF handling, and ept_handle_violation()

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -90,6 +90,15 @@ static bool_t amd_erratum383_found __rea
 static uint64_t osvw_length, osvw_status;
 static DEFINE_SPINLOCK(osvw_lock);
 
+/* Only crash the guest if the problem originates in kernel mode. */
+static void svm_crash_or_fault(struct vcpu *v)
+{
+    if ( vmcb_get_cpl(v->arch.hvm_svm.vmcb) )
+        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
+    else
+        domain_crash(v->domain);
+}
+
 void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len)
 {
     struct vcpu *curr = current;
@@ -100,7 +109,7 @@ void __update_guest_eip(struct cpu_user_
     if ( unlikely(inst_len > 15) )
     {
         gdprintk(XENLOG_ERR, "Bad instruction length %u\n", inst_len);
-        domain_crash(curr->domain);
+        svm_crash_or_fault(curr);
         return;
     }
 
@@ -2680,11 +2689,7 @@ void svm_vmexit_handler(struct cpu_user_
                  "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
                  exit_reason, 
                  (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
-        if ( vmcb_get_cpl(vmcb) )
-            hvm_inject_hw_exception(TRAP_invalid_op,
-                                    HVM_DELIVER_NO_ERROR_CODE);
-        else
-            domain_crash(v->domain);
+        svm_crash_or_fault(v);
         break;
     }
 
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -134,6 +134,18 @@ static void vmx_vcpu_destroy(struct vcpu
     passive_domain_destroy(v);
 }
 
+/* Only crash the guest if the problem originates in kernel mode. */
+static void vmx_crash_or_fault(struct vcpu *v)
+{
+    struct segment_register ss;
+
+    vmx_get_segment_register(v, x86_seg_ss, &ss);
+    if ( ss.attr.fields.dpl )
+        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
+    else
+        domain_crash(v->domain);
+}
+
 static DEFINE_PER_CPU(struct vmx_msr_state, host_msr_state);
 
 static const u32 msr_index[] =
@@ -2508,7 +2520,7 @@ static void vmx_failed_vmentry(unsigned 
     vmcs_dump_vcpu(curr);
     printk("**************************************\n");
 
-    domain_crash(curr->domain);
+    vmx_crash_or_fault(curr);
 }
 
 void vmx_enter_realmode(struct cpu_user_regs *regs)
@@ -3161,19 +3173,8 @@ void vmx_vmexit_handler(struct cpu_user_
     /* fall through */
     default:
     exit_and_crash:
-        {
-            struct segment_register ss;
-
-            gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n",
-                     exit_reason);
-
-            vmx_get_segment_register(v, x86_seg_ss, &ss);
-            if ( ss.attr.fields.dpl )
-                hvm_inject_hw_exception(TRAP_invalid_op,
-                                        HVM_DELIVER_NO_ERROR_CODE);
-            else
-                domain_crash(v->domain);
-        }
+        gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n", exit_reason);
+        vmx_crash_or_fault(v);
         break;
     }
 




[-- Attachment #2: x86-HVM-dont-crash-guest-in-user-mode.patch --]
[-- Type: text/plain, Size: 3580 bytes --]

x86/HVM: don't crash guest upon problems occurring in user mode

This extends commit 5283b310 ("x86/HVM: only kill guest when unknown VM
exit occurred in guest kernel mode") to a few more cases, including the
failed VM entry one that XSA-110 was needed to be issued for.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: - s/crash_or_gp/crash_or_fault/
    - drop changes to svm_do_nested_pgfault(), svm_vmexit_handler()'s
      VMEXIT_NPF handling, and ept_handle_violation()

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -90,6 +90,15 @@ static bool_t amd_erratum383_found __rea
 static uint64_t osvw_length, osvw_status;
 static DEFINE_SPINLOCK(osvw_lock);
 
+/* Only crash the guest if the problem originates in kernel mode. */
+static void svm_crash_or_fault(struct vcpu *v)
+{
+    if ( vmcb_get_cpl(v->arch.hvm_svm.vmcb) )
+        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
+    else
+        domain_crash(v->domain);
+}
+
 void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len)
 {
     struct vcpu *curr = current;
@@ -100,7 +109,7 @@ void __update_guest_eip(struct cpu_user_
     if ( unlikely(inst_len > 15) )
     {
         gdprintk(XENLOG_ERR, "Bad instruction length %u\n", inst_len);
-        domain_crash(curr->domain);
+        svm_crash_or_fault(curr);
         return;
     }
 
@@ -2680,11 +2689,7 @@ void svm_vmexit_handler(struct cpu_user_
                  "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
                  exit_reason, 
                  (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
-        if ( vmcb_get_cpl(vmcb) )
-            hvm_inject_hw_exception(TRAP_invalid_op,
-                                    HVM_DELIVER_NO_ERROR_CODE);
-        else
-            domain_crash(v->domain);
+        svm_crash_or_fault(v);
         break;
     }
 
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -134,6 +134,18 @@ static void vmx_vcpu_destroy(struct vcpu
     passive_domain_destroy(v);
 }
 
+/* Only crash the guest if the problem originates in kernel mode. */
+static void vmx_crash_or_fault(struct vcpu *v)
+{
+    struct segment_register ss;
+
+    vmx_get_segment_register(v, x86_seg_ss, &ss);
+    if ( ss.attr.fields.dpl )
+        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
+    else
+        domain_crash(v->domain);
+}
+
 static DEFINE_PER_CPU(struct vmx_msr_state, host_msr_state);
 
 static const u32 msr_index[] =
@@ -2508,7 +2520,7 @@ static void vmx_failed_vmentry(unsigned 
     vmcs_dump_vcpu(curr);
     printk("**************************************\n");
 
-    domain_crash(curr->domain);
+    vmx_crash_or_fault(curr);
 }
 
 void vmx_enter_realmode(struct cpu_user_regs *regs)
@@ -3161,19 +3173,8 @@ void vmx_vmexit_handler(struct cpu_user_
     /* fall through */
     default:
     exit_and_crash:
-        {
-            struct segment_register ss;
-
-            gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n",
-                     exit_reason);
-
-            vmx_get_segment_register(v, x86_seg_ss, &ss);
-            if ( ss.attr.fields.dpl )
-                hvm_inject_hw_exception(TRAP_invalid_op,
-                                        HVM_DELIVER_NO_ERROR_CODE);
-            else
-                domain_crash(v->domain);
-        }
+        gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n", exit_reason);
+        vmx_crash_or_fault(v);
         break;
     }
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 3/3] x86/HVM: don't crash guest upon problems occurring in user mode

[Tim Deegan]

At 16:37 +0100 on 20 Nov (1416497837), Jan Beulich wrote:
> This extends commit 5283b310 ("x86/HVM: only kill guest when unknown VM
> exit occurred in guest kernel mode") to a few more cases, including the
> failed VM entry one that XSA-110 was needed to be issued for.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Tim Deegan <tim@xen.org>

Re: [PATCH v2 3/3] x86/HVM: don't crash guest upon problems occurring in user mode

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 3853 bytes --]

On 20/11/14 15:37, Jan Beulich wrote:
> This extends commit 5283b310 ("x86/HVM: only kill guest when unknown VM
> exit occurred in guest kernel mode") to a few more cases, including the
> failed VM entry one that XSA-110 was needed to be issued for.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> v2: - s/crash_or_gp/crash_or_fault/
>     - drop changes to svm_do_nested_pgfault(), svm_vmexit_handler()'s
>       VMEXIT_NPF handling, and ept_handle_violation()
>
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -90,6 +90,15 @@ static bool_t amd_erratum383_found __rea
>  static uint64_t osvw_length, osvw_status;
>  static DEFINE_SPINLOCK(osvw_lock);
>  
> +/* Only crash the guest if the problem originates in kernel mode. */
> +static void svm_crash_or_fault(struct vcpu *v)
> +{
> +    if ( vmcb_get_cpl(v->arch.hvm_svm.vmcb) )
> +        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
> +    else
> +        domain_crash(v->domain);
> +}
> +
>  void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len)
>  {
>      struct vcpu *curr = current;
> @@ -100,7 +109,7 @@ void __update_guest_eip(struct cpu_user_
>      if ( unlikely(inst_len > 15) )
>      {
>          gdprintk(XENLOG_ERR, "Bad instruction length %u\n", inst_len);
> -        domain_crash(curr->domain);
> +        svm_crash_or_fault(curr);
>          return;
>      }
>  
> @@ -2680,11 +2689,7 @@ void svm_vmexit_handler(struct cpu_user_
>                   "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
>                   exit_reason, 
>                   (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
> -        if ( vmcb_get_cpl(vmcb) )
> -            hvm_inject_hw_exception(TRAP_invalid_op,
> -                                    HVM_DELIVER_NO_ERROR_CODE);
> -        else
> -            domain_crash(v->domain);
> +        svm_crash_or_fault(v);
>          break;
>      }
>  
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -134,6 +134,18 @@ static void vmx_vcpu_destroy(struct vcpu
>      passive_domain_destroy(v);
>  }
>  
> +/* Only crash the guest if the problem originates in kernel mode. */
> +static void vmx_crash_or_fault(struct vcpu *v)
> +{
> +    struct segment_register ss;
> +
> +    vmx_get_segment_register(v, x86_seg_ss, &ss);
> +    if ( ss.attr.fields.dpl )
> +        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
> +    else
> +        domain_crash(v->domain);
> +}
> +
>  static DEFINE_PER_CPU(struct vmx_msr_state, host_msr_state);
>  
>  static const u32 msr_index[] =
> @@ -2508,7 +2520,7 @@ static void vmx_failed_vmentry(unsigned 
>      vmcs_dump_vcpu(curr);
>      printk("**************************************\n");
>  
> -    domain_crash(curr->domain);
> +    vmx_crash_or_fault(curr);
>  }
>  
>  void vmx_enter_realmode(struct cpu_user_regs *regs)
> @@ -3161,19 +3173,8 @@ void vmx_vmexit_handler(struct cpu_user_
>      /* fall through */
>      default:
>      exit_and_crash:
> -        {
> -            struct segment_register ss;
> -
> -            gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n",
> -                     exit_reason);
> -
> -            vmx_get_segment_register(v, x86_seg_ss, &ss);
> -            if ( ss.attr.fields.dpl )
> -                hvm_inject_hw_exception(TRAP_invalid_op,
> -                                        HVM_DELIVER_NO_ERROR_CODE);
> -            else
> -                domain_crash(v->domain);
> -        }
> +        gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n", exit_reason);
> +        vmx_crash_or_fault(v);
>          break;
>      }
>  
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 4611 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 0/3] x86: XSA-109/110 follow-up (to be considered for 4.5)

[Jan Beulich]

>>> On 20.11.14 at 11:04, <JBeulich@suse.com> wrote:
> 1: tighten page table owner checking in do_mmu_update()
> 2: don't ignore foreigndom input on various MMUEXT ops
> 3: HVM: don't crash guest upon problems occurring in user mode
> 
> Reason to request considering this for 4.5: Tightened argument
> checking (as done in the first two patches) reduces the chances
> of them getting abused. Not unduly crashing the guest (as done
> in the third one) may avoid future security issues of guest user
> mode affecting the guest kernel.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Hi Konrad - looks like I forgot to Cc you...

Jan

Re: [PATCH 0/3] x86: XSA-109/110 follow-up (to be considered for 4.5)

[Konrad Rzeszutek Wilk]

On Mon, Nov 24, 2014 at 11:49:24AM +0000, Jan Beulich wrote:
> >>> On 20.11.14 at 11:04, <JBeulich@suse.com> wrote:
> > 1: tighten page table owner checking in do_mmu_update()
> > 2: don't ignore foreigndom input on various MMUEXT ops
> > 3: HVM: don't crash guest upon problems occurring in user mode
> > 
> > Reason to request considering this for 4.5: Tightened argument
> > checking (as done in the first two patches) reduces the chances
> > of them getting abused. Not unduly crashing the guest (as done
> > in the third one) may avoid future security issues of guest user
> > mode affecting the guest kernel.
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Hi Konrad - looks like I forgot to Cc you...

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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