| Message ID | 0be7501ace42d856b344828755ece18659dabd33.1587142844.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | VM forking | expand |
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@intel.com> > --- > 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.
On Mon, Apr 20, 2020 at 1:57 AM Roger Pau Monné <roger.pau@citrix.com> 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@intel.com> > > --- > > 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
On Mon, Apr 20, 2020 at 08:19:12AM -0600, Tamas K Lengyel wrote: > On Mon, Apr 20, 2020 at 1:57 AM Roger Pau Monné <roger.pau@citrix.com> wrote: > > > > On Fri, Apr 17, 2020 at 10:06:32AM -0700, Tamas K Lengyel wrote: > > > 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. Right, but when new flags are added it looks weird IMO to have: #define XENMEM_FORK_WITH_IOMMU_ALLOWED 1 #define XENMEM_FOO_BAR_WITH_FOO (1u << 1) Thanks, Roger.
On 17.04.2020 19:06, Tamas K Lengyel wrote: > @@ -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; Rather than outright dropping this, you now want to bail on any bits set in flags except the one that's currently defined. Jan
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)) ) 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; + + 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 */ + uint16_t flags; /* IN: optional settings */ + uint16_t pad[2]; /* Must be set to 0 */ } fork; } u; };
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 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. Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> --- xen/arch/x86/mm/mem_sharing.c | 18 ++++++++++++------ xen/include/public/memory.h | 4 +++- 2 files changed, 15 insertions(+), 7 deletions(-)