[PATCH for-4.14 1/2] x86/mem_sharing: Prohibit interrupt injection for forks

[PATCH for-4.14 1/2] x86/mem_sharing: Prohibit interrupt injection for forks

[Tamas K Lengyel]

When running shallow forks without device models it may be undesirable for Xen
to inject interrupts. With Windows forks we have observed the kernel going into
infinite loops when trying to process such interrupts. By disabling interrupt
injection the fuzzer can exercise the target code without interference.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
---
 xen/arch/x86/hvm/vmx/intr.c      | 4 ++++
 xen/arch/x86/mm/mem_sharing.c    | 6 +++++-
 xen/include/asm-x86/hvm/domain.h | 2 ++
 xen/include/public/memory.h      | 1 +
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 000e14af49..3814795e3f 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -256,6 +256,10 @@ void vmx_intr_assist(void)
     if ( unlikely(v->arch.vm_event) && v->arch.vm_event->sync_event )
         return;
 
+    /* Block event injection for VM fork if requested */
+    if ( unlikely(v->domain->arch.hvm.mem_sharing.prohibit_interrupts) )
+        return;
+
     /* Crank the handle on interrupt state. */
     pt_vector = pt_update_irq(v);
 
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 7271e5c90b..7352fce866 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -2106,7 +2106,8 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
         rc = -EINVAL;
         if ( mso.u.fork.pad )
             goto out;
-        if ( mso.u.fork.flags & ~XENMEM_FORK_WITH_IOMMU_ALLOWED )
+        if ( mso.u.fork.flags & ~(XENMEM_FORK_WITH_IOMMU_ALLOWED |
+                                  XENMEM_FORK_PROHIBIT_INTERRUPTS) )
             goto out;
 
         rc = rcu_lock_live_remote_domain_by_id(mso.u.fork.parent_domain,
@@ -2134,6 +2135,9 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
             rc = hypercall_create_continuation(__HYPERVISOR_memory_op,
                                                "lh", XENMEM_sharing_op,
                                                arg);
+        else if ( !rc && (mso.u.fork.flags & XENMEM_FORK_PROHIBIT_INTERRUPTS) )
+            d->arch.hvm.mem_sharing.prohibit_interrupts = true;
+
         rcu_unlock_domain(pd);
         break;
     }
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 95fe18cddc..e114f818d3 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -74,6 +74,8 @@ struct mem_sharing_domain
      * to resume the search.
      */
     unsigned long next_shared_gfn_to_relinquish;
+
+    bool prohibit_interrupts;
 };
 #endif
 
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index dbd35305df..fe2e6caa68 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -537,6 +537,7 @@ struct xen_mem_sharing_op {
         struct mem_sharing_op_fork {      /* OP_FORK */
             domid_t parent_domain;        /* IN: parent's domain id */
 #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
+#define XENMEM_FORK_PROHIBIT_INTERRUPTS (1u << 1)
             uint16_t flags;               /* IN: optional settings */
             uint32_t pad;                 /* Must be set to 0 */
         } fork;
-- 
2.25.1

[PATCH for-4.14 2/2] tools/libxc: xc_memshr_fork with interrupts disabled

[Tamas K Lengyel]

Toolstack side for creating forks with interrupt injection disabled.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
---
 tools/libxc/include/xenctrl.h | 3 ++-
 tools/libxc/xc_memshr.c       | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 45ff7db1e8..0ea839b72a 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2242,7 +2242,8 @@ int xc_memshr_range_share(xc_interface *xch,
 int xc_memshr_fork(xc_interface *xch,
                    uint32_t source_domain,
                    uint32_t client_domain,
-                   bool allow_with_iommu);
+                   bool allow_with_iommu,
+                   bool prohibit_interrupts);
 
 /*
  * Note: this function is only intended to be used on short-lived forks that
diff --git a/tools/libxc/xc_memshr.c b/tools/libxc/xc_memshr.c
index 2300cc7075..e2de1d3aa2 100644
--- a/tools/libxc/xc_memshr.c
+++ b/tools/libxc/xc_memshr.c
@@ -240,7 +240,7 @@ int xc_memshr_debug_gref(xc_interface *xch,
 }
 
 int xc_memshr_fork(xc_interface *xch, uint32_t pdomid, uint32_t domid,
-                   bool allow_with_iommu)
+                   bool allow_with_iommu, bool prohibit_interrupts)
 {
     xen_mem_sharing_op_t mso;
 
@@ -251,6 +251,8 @@ int xc_memshr_fork(xc_interface *xch, uint32_t pdomid, uint32_t domid,
 
     if ( allow_with_iommu )
         mso.u.fork.flags |= XENMEM_FORK_WITH_IOMMU_ALLOWED;
+    if ( prohibit_interrupts )
+        mso.u.fork.flags |= XENMEM_FORK_PROHIBIT_INTERRUPTS;
 
     return xc_memshr_memop(xch, domid, &mso);
 }
-- 
2.25.1

Re: [PATCH for-4.14 2/2] tools/libxc: xc_memshr_fork with interrupts disabled

[Roger Pau Monné]

On Thu, May 21, 2020 at 03:53:23PM -0700, Tamas K Lengyel wrote:
> Toolstack side for creating forks with interrupt injection disabled.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I have the same suggestion to use block instead of prohibit, so if you
agree to change it on patch #1 it should also be changed here.

Thanks, Roger.

Re: [PATCH for-4.14 2/2] tools/libxc: xc_memshr_fork with interrupts disabled

[Ian Jackson]

Tamas K Lengyel writes ("[PATCH for-4.14 2/2] tools/libxc: xc_memshr_fork with interrupts disabled"):
> Toolstack side for creating forks with interrupt injection disabled.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Subject to the hypervisor folks being happy with the underlying
feature.

Ian.

Re: [PATCH for-4.14 1/2] x86/mem_sharing: Prohibit interrupt injection for forks

[Tamas K Lengyel]

> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index 000e14af49..3814795e3f 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -256,6 +256,10 @@ void vmx_intr_assist(void)
>      if ( unlikely(v->arch.vm_event) && v->arch.vm_event->sync_event )
>          return;
>

Just noticed after sending the patch that this block needs to be wrapped in

#ifdef CONFIG_MEM_SHARING

> +    /* Block event injection for VM fork if requested */
> +    if ( unlikely(v->domain->arch.hvm.mem_sharing.prohibit_interrupts) )
> +        return;

#endif

> +
>      /* Crank the handle on interrupt state. */
>      pt_vector = pt_update_irq(v);
>

I can resend if necessary but its also a trivial fixup when applying
so let me know what would be preferred. I pushed the fixed-up version
to http://xenbits.xen.org/gitweb/?p=people/tklengyel/xen.git;a=shortlog;h=refs/heads/fork_interrupts.

Thanks,
Tamas

Re: [PATCH for-4.14 1/2] x86/mem_sharing: Prohibit interrupt injection for forks

[Roger Pau Monné]

On Thu, May 21, 2020 at 03:53:22PM -0700, Tamas K Lengyel wrote:
> When running shallow forks without device models it may be undesirable for Xen
> to inject interrupts. With Windows forks we have observed the kernel going into
> infinite loops when trying to process such interrupts. By disabling interrupt
> injection the fuzzer can exercise the target code without interference.

Could you add some more information about why windows goes into
infinite loops? Is it trying to access MMIO regions of emulated
devices and getting ~0 out of them instead of the expected data and
that causes it to loop indefinitely?

> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
> ---
>  xen/arch/x86/hvm/vmx/intr.c      | 4 ++++
>  xen/arch/x86/mm/mem_sharing.c    | 6 +++++-
>  xen/include/asm-x86/hvm/domain.h | 2 ++
>  xen/include/public/memory.h      | 1 +
>  4 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index 000e14af49..3814795e3f 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c

I think you are missing the AMD side of this change? A similar
adjustment should be done to svm_intr_assist, or else it should be
noted in the commit message the reason this is Intel only.

> @@ -256,6 +256,10 @@ void vmx_intr_assist(void)
>      if ( unlikely(v->arch.vm_event) && v->arch.vm_event->sync_event )
>          return;
>  
> +    /* Block event injection for VM fork if requested */
> +    if ( unlikely(v->domain->arch.hvm.mem_sharing.prohibit_interrupts) )
> +        return;
> +
>      /* Crank the handle on interrupt state. */
>      pt_vector = pt_update_irq(v);
>  
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index 7271e5c90b..7352fce866 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -2106,7 +2106,8 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>          rc = -EINVAL;
>          if ( mso.u.fork.pad )
>              goto out;
> -        if ( mso.u.fork.flags & ~XENMEM_FORK_WITH_IOMMU_ALLOWED )
> +        if ( mso.u.fork.flags & ~(XENMEM_FORK_WITH_IOMMU_ALLOWED |
> +                                  XENMEM_FORK_PROHIBIT_INTERRUPTS) )


Nit: I would move the XENMEM_FORK_ option ORing to a newline, so that
you don't have to use a whole line every time a new option is used.
Ie:

        if ( mso.u.fork.flags &
             ~(XENMEM_FORK_WITH_IOMMU_ALLOWED | XENMEM_FORK_BLOCK_INTERRUPTS) )


>              goto out;
>  
>          rc = rcu_lock_live_remote_domain_by_id(mso.u.fork.parent_domain,
> @@ -2134,6 +2135,9 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>              rc = hypercall_create_continuation(__HYPERVISOR_memory_op,
>                                                 "lh", XENMEM_sharing_op,
>                                                 arg);
> +        else if ( !rc && (mso.u.fork.flags & XENMEM_FORK_PROHIBIT_INTERRUPTS) )
> +            d->arch.hvm.mem_sharing.prohibit_interrupts = true;
> +
>          rcu_unlock_domain(pd);
>          break;
>      }
> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
> index 95fe18cddc..e114f818d3 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -74,6 +74,8 @@ struct mem_sharing_domain
>       * to resume the search.
>       */
>      unsigned long next_shared_gfn_to_relinquish;
> +
> +    bool prohibit_interrupts;

Nit: I would prefer block_interrupts, prohibit seems very formal to
me, but I'm not a native speaker, so feel free to ignore this
suggestion.

>  };
>  #endif
>  
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index dbd35305df..fe2e6caa68 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -537,6 +537,7 @@ struct xen_mem_sharing_op {
>          struct mem_sharing_op_fork {      /* OP_FORK */
>              domid_t parent_domain;        /* IN: parent's domain id */
>  #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
> +#define XENMEM_FORK_PROHIBIT_INTERRUPTS (1u << 1)

FWIW, I would also use BLOCK here instead of PROHIBIT.

Thanks, Roger.