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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Apr 20, 2020 at 1:57 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> 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?

Correct, this mode would only be for forks that have no access to
devices at all (--launch-dm no).

>
> >
> > 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)

Ack

>
> >              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?

Sure.

>
> 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).

I was thinking of doing that then it won't fit into a single line. For
this particular flag it would also make no difference.

>
> > +            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.

True.

Thanks,
Tamas