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

[PATCH v2 for-4.14 1/2] x86/mem_sharing: block 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, likely because it attempts
to interact with devices that are not responding without QEMU running. By
disabling interrupt injection the fuzzer can exercise the target code without
interference.

Forks & memory sharing are only available on Intel CPUs so this only applies
to vmx.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
---
v2: prohibit => block
    minor style adjustments
---
 xen/arch/x86/hvm/vmx/intr.c      | 6 ++++++
 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, 14 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 000e14af49..80bfbb4787 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -256,6 +256,12 @@ void vmx_intr_assist(void)
     if ( unlikely(v->arch.vm_event) && v->arch.vm_event->sync_event )
         return;
 
+#ifdef CONFIG_MEM_SHARING
+    /* Block event injection for VM fork if requested */
+    if ( unlikely(v->domain->arch.hvm.mem_sharing.block_interrupts) )
+        return;
+#endif
+
     /* 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..0c45a8d67e 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_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_BLOCK_INTERRUPTS) )
+            d->arch.hvm.mem_sharing.block_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..37e494d234 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 block_interrupts;
 };
 #endif
 
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index dbd35305df..1e4959638d 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_BLOCK_INTERRUPTS   (1u << 1)
             uint16_t flags;               /* IN: optional settings */
             uint32_t pad;                 /* Must be set to 0 */
         } fork;
-- 
2.25.1

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

[Tamas K Lengyel]

Toolstack side for creating forks with interrupt injection blocked.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.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..804ff001d7 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 block_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..a6cfd7dccf 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 block_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 ( block_interrupts )
+        mso.u.fork.flags |= XENMEM_FORK_BLOCK_INTERRUPTS;
 
     return xc_memshr_memop(xch, domid, &mso);
 }
-- 
2.25.1

RE: [PATCH v2 for-4.14 1/2] x86/mem_sharing: block interrupt injection for forks

[Tian, Kevin]

> From: Lengyel, Tamas <tamas.lengyel@intel.com>
> Sent: Saturday, May 23, 2020 12:34 AM
> 
> When running shallow forks without device models it may be undesirable for
> Xen

what is shallow forks? and why interrupt injection is not desired without
device model? If it means just without Qemu thing, you still get local APIC
interrupts such as timers, PMI, etc.

> to inject interrupts. With Windows forks we have observed the kernel going
> into
> infinite loops when trying to process such interrupts, likely because it
> attempts

what is the relationship between shallow forks and windows forks then?

> to interact with devices that are not responding without QEMU running. By
> disabling interrupt injection the fuzzer can exercise the target code without
> interference.

what is the fuzzer?

> 
> Forks & memory sharing are only available on Intel CPUs so this only applies
> to vmx.

I feel lots of background is missing thus difficult to judge whether below change
is desired...

> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
> ---
> v2: prohibit => block
>     minor style adjustments
> ---
>  xen/arch/x86/hvm/vmx/intr.c      | 6 ++++++
>  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, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index 000e14af49..80bfbb4787 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -256,6 +256,12 @@ void vmx_intr_assist(void)
>      if ( unlikely(v->arch.vm_event) && v->arch.vm_event->sync_event )
>          return;
> 
> +#ifdef CONFIG_MEM_SHARING
> +    /* Block event injection for VM fork if requested */
> +    if ( unlikely(v->domain->arch.hvm.mem_sharing.block_interrupts) )
> +        return;
> +#endif
> +
>      /* 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..0c45a8d67e 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_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_BLOCK_INTERRUPTS) )
> +            d->arch.hvm.mem_sharing.block_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..37e494d234 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 block_interrupts;
>  };
>  #endif
> 
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index dbd35305df..1e4959638d 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_BLOCK_INTERRUPTS   (1u << 1)
>              uint16_t flags;               /* IN: optional settings */
>              uint32_t pad;                 /* Must be set to 0 */
>          } fork;
> --
> 2.25.1

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

[Tamas K Lengyel]

On Sun, May 24, 2020 at 8:33 PM Tian, Kevin <kevin.tian@intel.com> wrote:
>
> > From: Lengyel, Tamas <tamas.lengyel@intel.com>
> > Sent: Saturday, May 23, 2020 12:34 AM
> >
> > When running shallow forks without device models it may be undesirable for
> > Xen
>
> what is shallow forks? and why interrupt injection is not desired without
> device model? If it means just without Qemu thing, you still get local APIC
> interrupts such as timers, PMI, etc.

I refer to shallow forks as VM forks that run without a device model
(ie. QEMU). Effectively these are domains that run only with CPU and
memory, both of which are copied from the parent VM as needed. When an
interrupt is injected into a VM fork (because its state is copied from
a parent where an interrupt might be pending) the interrupt handler
might want to talk to the device model which is not present for the
fork. In such situations the VM fork ends up executing the interrupt
handler instead of the code we want to fuzz, which we want to avoid
for obvious reasons.

>
> > to inject interrupts. With Windows forks we have observed the kernel going
> > into
> > infinite loops when trying to process such interrupts, likely because it
> > attempts
>
> what is the relationship between shallow forks and windows forks then?

They are the same, but we only observed this behavior with Windows forks.

>
> > to interact with devices that are not responding without QEMU running. By
> > disabling interrupt injection the fuzzer can exercise the target code without
> > interference.
>
> what is the fuzzer?

https://github.com/intel/kernel-fuzzer-for-xen-project/

>
> >
> > Forks & memory sharing are only available on Intel CPUs so this only applies
> > to vmx.
>
> I feel lots of background is missing thus difficult to judge whether below change
> is desired...

You may find the VM forking series worthwhile to review to get some
context: https://lists.xenproject.org/archives/html/xen-devel/2020-04/msg01162.html.
In a nutshell, it's an experimental feature geared towards fuzzing and
it's disabled by default (note that it's gated on CONFIG_MEM_SHARING
being enabled).

Tamas

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

[Jan Beulich]

On 22.05.2020 18:33, 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, likely because it attempts
> to interact with devices that are not responding without QEMU running. By
> disabling interrupt injection the fuzzer can exercise the target code without
> interference.
> 
> Forks & memory sharing are only available on Intel CPUs so this only applies
> to vmx.

Looking at e.g. mem_sharing_control() I can't seem to be able to confirm
this. Would you mind pointing me at where this restriction is coming from?

> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -256,6 +256,12 @@ void vmx_intr_assist(void)
>      if ( unlikely(v->arch.vm_event) && v->arch.vm_event->sync_event )
>          return;
>  
> +#ifdef CONFIG_MEM_SHARING
> +    /* Block event injection for VM fork if requested */
> +    if ( unlikely(v->domain->arch.hvm.mem_sharing.block_interrupts) )
> +        return;
> +#endif

The two earlier returns are temporary as far as the guest is concerned,
i.e. eventually the interrupt(s) will get delivered. The one you add
looks as if it is a permanent thing, i.e. interrupt requests will pile
up and potentially confuse a guest down the road. This _may_ be okay
for your short-lived-shallow-fork scenario, but then wants at least
calling out in the public header by a comment (and I think the same
goes for XENMEM_FORK_WITH_IOMMU_ALLOWED that's already there).

> --- 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 block_interrupts;
>  };

Please can you avoid unnecessary growth of the structure by inserting
next to the pre-existing bool rather than at the end?

Jan

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

[Tamas K Lengyel]

On Mon, May 25, 2020 at 12:06 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 22.05.2020 18:33, 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, likely because it attempts
> > to interact with devices that are not responding without QEMU running. By
> > disabling interrupt injection the fuzzer can exercise the target code without
> > interference.
> >
> > Forks & memory sharing are only available on Intel CPUs so this only applies
> > to vmx.
>
> Looking at e.g. mem_sharing_control() I can't seem to be able to confirm
> this. Would you mind pointing me at where this restriction is coming from?

Both mem_access and mem_sharing are only implemented for EPT:
http://xenbits.xen.org/hg/xen-unstable.hg/file/5eadf9363c25/xen/arch/x86/mm/p2m-ept.c#l126.

>
> > --- a/xen/arch/x86/hvm/vmx/intr.c
> > +++ b/xen/arch/x86/hvm/vmx/intr.c
> > @@ -256,6 +256,12 @@ void vmx_intr_assist(void)
> >      if ( unlikely(v->arch.vm_event) && v->arch.vm_event->sync_event )
> >          return;
> >
> > +#ifdef CONFIG_MEM_SHARING
> > +    /* Block event injection for VM fork if requested */
> > +    if ( unlikely(v->domain->arch.hvm.mem_sharing.block_interrupts) )
> > +        return;
> > +#endif
>
> The two earlier returns are temporary as far as the guest is concerned,
> i.e. eventually the interrupt(s) will get delivered. The one you add
> looks as if it is a permanent thing, i.e. interrupt requests will pile
> up and potentially confuse a guest down the road. This _may_ be okay
> for your short-lived-shallow-fork scenario, but then wants at least
> calling out in the public header by a comment (and I think the same
> goes for XENMEM_FORK_WITH_IOMMU_ALLOWED that's already there).

This is indeed only for the short-lived forks, that's why this is an
optional flag that can be enabled when creating forks and it's not on
by default. In that use-case the VM executes for fractions of a second
and we want to only executes very specific code-segments with
absolutely no interference. Interrupts in that case are just a
nuisance that in the best case slow the fuzzing process down but as we
observed in the worst case complete stall it.

>
> > --- 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 block_interrupts;
> >  };
>
> Please can you avoid unnecessary growth of the structure by inserting
> next to the pre-existing bool rather than at the end?

Sure. Do you want me to resend the patch for that?

Tamas

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

[Tamas K Lengyel]

On Mon, May 25, 2020 at 6:18 AM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>
> On Mon, May 25, 2020 at 12:06 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 22.05.2020 18:33, 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, likely because it attempts
> > > to interact with devices that are not responding without QEMU running. By
> > > disabling interrupt injection the fuzzer can exercise the target code without
> > > interference.
> > >
> > > Forks & memory sharing are only available on Intel CPUs so this only applies
> > > to vmx.
> >
> > Looking at e.g. mem_sharing_control() I can't seem to be able to confirm
> > this. Would you mind pointing me at where this restriction is coming from?
>
> Both mem_access and mem_sharing are only implemented for EPT:
> http://xenbits.xen.org/hg/xen-unstable.hg/file/5eadf9363c25/xen/arch/x86/mm/p2m-ept.c#l126.
>
> >
> > > --- a/xen/arch/x86/hvm/vmx/intr.c
> > > +++ b/xen/arch/x86/hvm/vmx/intr.c
> > > @@ -256,6 +256,12 @@ void vmx_intr_assist(void)
> > >      if ( unlikely(v->arch.vm_event) && v->arch.vm_event->sync_event )
> > >          return;
> > >
> > > +#ifdef CONFIG_MEM_SHARING
> > > +    /* Block event injection for VM fork if requested */
> > > +    if ( unlikely(v->domain->arch.hvm.mem_sharing.block_interrupts) )
> > > +        return;
> > > +#endif
> >
> > The two earlier returns are temporary as far as the guest is concerned,
> > i.e. eventually the interrupt(s) will get delivered. The one you add
> > looks as if it is a permanent thing, i.e. interrupt requests will pile
> > up and potentially confuse a guest down the road. This _may_ be okay
> > for your short-lived-shallow-fork scenario, but then wants at least
> > calling out in the public header by a comment (and I think the same
> > goes for XENMEM_FORK_WITH_IOMMU_ALLOWED that's already there).
>
> This is indeed only for the short-lived forks, that's why this is an
> optional flag that can be enabled when creating forks and it's not on
> by default. In that use-case the VM executes for fractions of a second
> and we want to only executes very specific code-segments with
> absolutely no interference. Interrupts in that case are just a
> nuisance that in the best case slow the fuzzing process down but as we
> observed in the worst case complete stall it.
>
> >
> > > --- 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 block_interrupts;
> > >  };
> >
> > Please can you avoid unnecessary growth of the structure by inserting
> > next to the pre-existing bool rather than at the end?
>
> Sure. Do you want me to resend the patch for that?

I'll just resend it anyway with the requested comments in the public header.

Tamas

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

[Jan Beulich]

On 25.05.2020 14:18, Tamas K Lengyel wrote:
> On Mon, May 25, 2020 at 12:06 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 22.05.2020 18:33, 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, likely because it attempts
>>> to interact with devices that are not responding without QEMU running. By
>>> disabling interrupt injection the fuzzer can exercise the target code without
>>> interference.
>>>
>>> Forks & memory sharing are only available on Intel CPUs so this only applies
>>> to vmx.
>>
>> Looking at e.g. mem_sharing_control() I can't seem to be able to confirm
>> this. Would you mind pointing me at where this restriction is coming from?
> 
> Both mem_access and mem_sharing are only implemented for EPT:
> http://xenbits.xen.org/hg/xen-unstable.hg/file/5eadf9363c25/xen/arch/x86/mm/p2m-ept.c#l126.

p2m-pt.c:p2m_type_to_flags() has a similar case label. And I can't
spot a respective restriction in mem_sharing_memop(), i.e. it looks
to me as if enabling mem-sharing on NPT (to satisfy hap_enabled()
in mem_sharing_control()) would be possible.

Jan

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

[Tamas K Lengyel]

On Mon, May 25, 2020 at 7:06 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.05.2020 14:18, Tamas K Lengyel wrote:
> > On Mon, May 25, 2020 at 12:06 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 22.05.2020 18:33, 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, likely because it attempts
> >>> to interact with devices that are not responding without QEMU running. By
> >>> disabling interrupt injection the fuzzer can exercise the target code without
> >>> interference.
> >>>
> >>> Forks & memory sharing are only available on Intel CPUs so this only applies
> >>> to vmx.
> >>
> >> Looking at e.g. mem_sharing_control() I can't seem to be able to confirm
> >> this. Would you mind pointing me at where this restriction is coming from?
> >
> > Both mem_access and mem_sharing are only implemented for EPT:
> > http://xenbits.xen.org/hg/xen-unstable.hg/file/5eadf9363c25/xen/arch/x86/mm/p2m-ept.c#l126.
>
> p2m-pt.c:p2m_type_to_flags() has a similar case label.

It doesn't do anything though, does it? For mem_sharing to work you
actively have to restrict the memory permissions on the shared entries
to be read/execute only. That's only done for EPT.

> And I can't
> spot a respective restriction in mem_sharing_memop(), i.e. it looks
> to me as if enabling mem-sharing on NPT (to satisfy hap_enabled()
> in mem_sharing_control()) would be possible.

If you are looking for an explicit gate like that, then you are right,
there isn't one. You can ask the original authors of this subsystem
why that is. If you feel like adding an extra gate, I wouldn't object.

Tamas

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

[Jan Beulich]

On 25.05.2020 15:46, Tamas K Lengyel wrote:
> On Mon, May 25, 2020 at 7:06 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 25.05.2020 14:18, Tamas K Lengyel wrote:
>>> On Mon, May 25, 2020 at 12:06 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 22.05.2020 18:33, 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, likely because it attempts
>>>>> to interact with devices that are not responding without QEMU running. By
>>>>> disabling interrupt injection the fuzzer can exercise the target code without
>>>>> interference.
>>>>>
>>>>> Forks & memory sharing are only available on Intel CPUs so this only applies
>>>>> to vmx.
>>>>
>>>> Looking at e.g. mem_sharing_control() I can't seem to be able to confirm
>>>> this. Would you mind pointing me at where this restriction is coming from?
>>>
>>> Both mem_access and mem_sharing are only implemented for EPT:
>>> http://xenbits.xen.org/hg/xen-unstable.hg/file/5eadf9363c25/xen/arch/x86/mm/p2m-ept.c#l126.
>>
>> p2m-pt.c:p2m_type_to_flags() has a similar case label.
> 
> It doesn't do anything though, does it? For mem_sharing to work you
> actively have to restrict the memory permissions on the shared entries
> to be read/execute only. That's only done for EPT.

Does it not? I seems to me that it does, seeing the case sits
together with the p2m_ram_ro and p2m_ram_logdirty ones:

    case p2m_ram_ro:
    case p2m_ram_logdirty:
    case p2m_ram_shared:
        return flags | P2M_BASE_FLAGS;

>> And I can't
>> spot a respective restriction in mem_sharing_memop(), i.e. it looks
>> to me as if enabling mem-sharing on NPT (to satisfy hap_enabled()
>> in mem_sharing_control()) would be possible.
> 
> If you are looking for an explicit gate like that, then you are right,
> there isn't one. You can ask the original authors of this subsystem
> why that is. If you feel like adding an extra gate, I wouldn't object.

Well, the question here isn't about gating - that's an independent
bug if it's indeed missing. The question is whether SVM code also
needs touching, as was previously requested. You tried to address
this by stating an Intel-only limitation, which I couldn't find
proof for (so far).

Jan

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

[Tamas K Lengyel]

On Mon, May 25, 2020 at 8:06 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.05.2020 15:46, Tamas K Lengyel wrote:
> > On Mon, May 25, 2020 at 7:06 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 25.05.2020 14:18, Tamas K Lengyel wrote:
> >>> On Mon, May 25, 2020 at 12:06 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 22.05.2020 18:33, 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, likely because it attempts
> >>>>> to interact with devices that are not responding without QEMU running. By
> >>>>> disabling interrupt injection the fuzzer can exercise the target code without
> >>>>> interference.
> >>>>>
> >>>>> Forks & memory sharing are only available on Intel CPUs so this only applies
> >>>>> to vmx.
> >>>>
> >>>> Looking at e.g. mem_sharing_control() I can't seem to be able to confirm
> >>>> this. Would you mind pointing me at where this restriction is coming from?
> >>>
> >>> Both mem_access and mem_sharing are only implemented for EPT:
> >>> http://xenbits.xen.org/hg/xen-unstable.hg/file/5eadf9363c25/xen/arch/x86/mm/p2m-ept.c#l126.
> >>
> >> p2m-pt.c:p2m_type_to_flags() has a similar case label.
> >
> > It doesn't do anything though, does it? For mem_sharing to work you
> > actively have to restrict the memory permissions on the shared entries
> > to be read/execute only. That's only done for EPT.
>
> Does it not? I seems to me that it does, seeing the case sits
> together with the p2m_ram_ro and p2m_ram_logdirty ones:
>
>     case p2m_ram_ro:
>     case p2m_ram_logdirty:
>     case p2m_ram_shared:
>         return flags | P2M_BASE_FLAGS;
>
> >> And I can't
> >> spot a respective restriction in mem_sharing_memop(), i.e. it looks
> >> to me as if enabling mem-sharing on NPT (to satisfy hap_enabled()
> >> in mem_sharing_control()) would be possible.
> >
> > If you are looking for an explicit gate like that, then you are right,
> > there isn't one. You can ask the original authors of this subsystem
> > why that is. If you feel like adding an extra gate, I wouldn't object.
>
> Well, the question here isn't about gating - that's an independent
> bug if it's indeed missing. The question is whether SVM code also
> needs touching, as was previously requested. You tried to address
> this by stating an Intel-only limitation, which I couldn't find
> proof for (so far).

Well, as far as I'm concerned VM forking is for Intel hardware only.
If mem_sharing seems to work for non-Intel hw - I was unaware of that
- than I'll just add an extra check for the VM fork hypercall that
gates it. It may be possible for technically be made available for
other hw as well, but at this time that's completely out-of-scope.

Tamas

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

[Tamas K Lengyel]

On Mon, May 25, 2020 at 8:14 AM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>
> On Mon, May 25, 2020 at 8:06 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 25.05.2020 15:46, Tamas K Lengyel wrote:
> > > On Mon, May 25, 2020 at 7:06 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>
> > >> On 25.05.2020 14:18, Tamas K Lengyel wrote:
> > >>> On Mon, May 25, 2020 at 12:06 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>
> > >>>> On 22.05.2020 18:33, 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, likely because it attempts
> > >>>>> to interact with devices that are not responding without QEMU running. By
> > >>>>> disabling interrupt injection the fuzzer can exercise the target code without
> > >>>>> interference.
> > >>>>>
> > >>>>> Forks & memory sharing are only available on Intel CPUs so this only applies
> > >>>>> to vmx.
> > >>>>
> > >>>> Looking at e.g. mem_sharing_control() I can't seem to be able to confirm
> > >>>> this. Would you mind pointing me at where this restriction is coming from?
> > >>>
> > >>> Both mem_access and mem_sharing are only implemented for EPT:
> > >>> http://xenbits.xen.org/hg/xen-unstable.hg/file/5eadf9363c25/xen/arch/x86/mm/p2m-ept.c#l126.
> > >>
> > >> p2m-pt.c:p2m_type_to_flags() has a similar case label.
> > >
> > > It doesn't do anything though, does it? For mem_sharing to work you
> > > actively have to restrict the memory permissions on the shared entries
> > > to be read/execute only. That's only done for EPT.
> >
> > Does it not? I seems to me that it does, seeing the case sits
> > together with the p2m_ram_ro and p2m_ram_logdirty ones:
> >
> >     case p2m_ram_ro:
> >     case p2m_ram_logdirty:
> >     case p2m_ram_shared:
> >         return flags | P2M_BASE_FLAGS;
> >
> > >> And I can't
> > >> spot a respective restriction in mem_sharing_memop(), i.e. it looks
> > >> to me as if enabling mem-sharing on NPT (to satisfy hap_enabled()
> > >> in mem_sharing_control()) would be possible.
> > >
> > > If you are looking for an explicit gate like that, then you are right,
> > > there isn't one. You can ask the original authors of this subsystem
> > > why that is. If you feel like adding an extra gate, I wouldn't object.
> >
> > Well, the question here isn't about gating - that's an independent
> > bug if it's indeed missing. The question is whether SVM code also
> > needs touching, as was previously requested. You tried to address
> > this by stating an Intel-only limitation, which I couldn't find
> > proof for (so far).
>
> Well, as far as I'm concerned VM forking is for Intel hardware only.
> If mem_sharing seems to work for non-Intel hw - I was unaware of that
> - than I'll just add an extra check for the VM fork hypercall that
> gates it. It may be possible for technically be made available for
> other hw as well, but at this time that's completely out-of-scope.

Actually, I'm going to just add that gate completely for mem_sharing.
Even if it at some time worked on other architectures (doubtful) at
this time its a usecase that's completely abandoned and forgotten and
as far as I'm concerned unmaintained with no plans from my side to
ever maintain it.

Tamas