Re: [PATCH v15 2/3] mem_sharing: allow forking domain with IOMMU enabled

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Apr 17, 2020 at 10:06:32AM -0700, Tamas K Lengyel wrote:
> The memory sharing subsystem by default doesn't allow a domain to share memory
> if it has an IOMMU active for obvious security reasons. However, when fuzzing 
> a
> VM fork, the same security restrictions don't necessarily apply. While it 
> makes
> no sense to try to create a full fork of a VM that has an IOMMU attached as 
> only
> one domain can own the pass-through device at a time, creating a shallow fork
> without a device model is still very useful for fuzzing kernel-mode drivers.
> 
> By allowing the parent VM to initialize the kernel-mode driver with a real
> device that's pass-through, the driver can enter into a state more suitable 
> for
                ^ passed
> fuzzing. Some of these initialization steps are quite complex and are easier 
> to
> perform when a real device is present. After the initialization, shallow forks
> can be utilized for fuzzing code-segments in the device driver that don't
> directly interact with the device.

If I understand this correctly, the forked VM won't have an IOMMU
setup, and the only domain allowed to interact with the device would
be the parent?

> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> ---
>  xen/arch/x86/mm/mem_sharing.c | 18 ++++++++++++------
>  xen/include/public/memory.h   |  4 +++-
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index 4b31a8b8f6..a5063d5992 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1430,7 +1430,8 @@ static int range_share(struct domain *d, struct domain 
> *cd,
>      return rc;
>  }
>  
> -static inline int mem_sharing_control(struct domain *d, bool enable)
> +static inline int mem_sharing_control(struct domain *d, bool enable,
> +                                      bool allow_iommu)
>  {
>      if ( enable )
>      {
> @@ -1440,7 +1441,7 @@ static inline int mem_sharing_control(struct domain *d, 
> bool enable)
>          if ( unlikely(!hap_enabled(d)) )
>              return -ENODEV;
>  
> -        if ( unlikely(is_iommu_enabled(d)) )
> +        if (unlikely(!allow_iommu && is_iommu_enabled(d)) )

Coding style (missing space)

>              return -EXDEV;
>      }
>  
> @@ -1827,7 +1828,8 @@ int 
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>      if ( rc )
>          goto out;
>  
> -    if ( !mem_sharing_enabled(d) && (rc = mem_sharing_control(d, true)) )
> +    if ( !mem_sharing_enabled(d) &&
> +         (rc = mem_sharing_control(d, true, false)) )
>          return rc;
>  
>      switch ( mso.op )
> @@ -2063,9 +2065,10 @@ int 
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>      case XENMEM_sharing_op_fork:
>      {
>          struct domain *pd;
> +        bool allow_iommu;
>  
>          rc = -EINVAL;
> -        if ( mso.u.fork.pad[0] || mso.u.fork.pad[1] || mso.u.fork.pad[2] )
> +        if ( mso.u.fork.pad[0] || mso.u.fork.pad[1] )
>              goto out;
>  
>          rc = rcu_lock_live_remote_domain_by_id(mso.u.fork.parent_domain,
> @@ -2080,7 +2083,10 @@ int 
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>              goto out;
>          }
>  
> -        if ( !mem_sharing_enabled(pd) && (rc = mem_sharing_control(pd, 
> true)) )
> +        allow_iommu = mso.u.fork.flags & XENMEM_FORK_WITH_IOMMU_ALLOWED;

I'm not sure whether it is worth to extract the flags into booleans at
this level. As you add more flags it might make sense to just pass the
whole flags field to mem_sharing_control?

mem_sharing_memop itself doesn't need to know which flags are
specified.

> +
> +        if ( !mem_sharing_enabled(pd) &&
> +             (rc = mem_sharing_control(pd, true, allow_iommu)) )
>          {
>              rcu_unlock_domain(pd);
>              goto out;
> @@ -2138,7 +2144,7 @@ int mem_sharing_domctl(struct domain *d, struct 
> xen_domctl_mem_sharing_op *mec)
>      switch ( mec->op )
>      {
>      case XEN_DOMCTL_MEM_SHARING_CONTROL:
> -        rc = mem_sharing_control(d, mec->u.enable);
> +        rc = mem_sharing_control(d, mec->u.enable, false);
>          break;
>  
>      default:
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index d36d64b8dc..1d2149def3 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -536,7 +536,9 @@ struct xen_mem_sharing_op {
>          } debug;
>          struct mem_sharing_op_fork {      /* OP_FORK */
>              domid_t parent_domain;        /* IN: parent's domain id */
> -            uint16_t pad[3];              /* Must be set to 0 */
> +#define XENMEM_FORK_WITH_IOMMU_ALLOWED 1  /* Allow forking domain with IOMMU 
> */

Since this is a flags field, can you express this is as: (1u << 0).

> +            uint16_t flags;               /* IN: optional settings */
> +            uint16_t pad[2];              /* Must be set to 0 */

Nit: padding can be made a uint32_t now instead of an array of two
uint16_t.

Or add an uint16_t between parent_domain and flags and make flags an
uint32_t.

Thanks, Roger.