diff mbox series

[v1,1/1] vfio: fix partial unmap check

Message ID 8079312ba39435a0ac92e084cc1a3fe291008a47.1635254797.git.anatoly.burakov@intel.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers show
Series [v1,1/1] vfio: fix partial unmap check | expand

Checks

Context Check Description
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Functional fail Functional Testing issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing fail Testing issues
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/checkpatch warning coding style issues

Commit Message

Anatoly Burakov Oct. 26, 2021, 1:26 p.m. UTC
Partial unmap support was introduced in commit c13ca4e81cac, and with it
was added a check that dereferenced the IOMMU type to determine whether
partial ummapping is supported for currently configured IOMMU type. In
certain circumstances (such as when VFIO is supported, but no devices
were bound to the VFIO driver), the IOMMU type pointer can be NULL.

However, dereferencing of IOMMU type was guarded by access to the user
maps list - that is, we were always checking the user map list first,
and then, if we found a memory region that encloses the one we're trying
to unmap, we would have performed the IOMMU type check.

This ensured that the IOMMU type check will not cause any NULL pointer
dereferences, because in order for an IOMMU type check to have been
performed, there necessarily must have been at least one memory region
that was previously mapped successfully, and that implies having a
defined IOMMU type.

When 56259f7fc010 was introduced, the IOMMU type check was moved to
before we were traversing the user mem maps list, thereby introducing a
potential NULL dereference, because the IOMMU type access was no longer
guarded by the user mem maps list traversal.

Fix the issue by moving the IOMMU type check to after the user mem maps
traversal, thereby ensuring that by the time the check happens, the
IOMMU type is always valid.

Fixes: 56259f7fc010 ("vfio: allow partially unmapping adjacent memory")
Cc: xuan.ding@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/eal/linux/eal_vfio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

David Marchand Oct. 27, 2021, 2:49 p.m. UTC | #1
On Tue, Oct 26, 2021 at 3:30 PM Anatoly Burakov
<anatoly.burakov@intel.com> wrote:
>
> Partial unmap support was introduced in commit c13ca4e81cac, and with it
> was added a check that dereferenced the IOMMU type to determine whether
> partial ummapping is supported for currently configured IOMMU type. In
> certain circumstances (such as when VFIO is supported, but no devices
> were bound to the VFIO driver), the IOMMU type pointer can be NULL.
>
> However, dereferencing of IOMMU type was guarded by access to the user
> maps list - that is, we were always checking the user map list first,
> and then, if we found a memory region that encloses the one we're trying
> to unmap, we would have performed the IOMMU type check.
>
> This ensured that the IOMMU type check will not cause any NULL pointer
> dereferences, because in order for an IOMMU type check to have been
> performed, there necessarily must have been at least one memory region
> that was previously mapped successfully, and that implies having a
> defined IOMMU type.
>
> When 56259f7fc010 was introduced, the IOMMU type check was moved to
> before we were traversing the user mem maps list, thereby introducing a
> potential NULL dereference, because the IOMMU type access was no longer
> guarded by the user mem maps list traversal.
>
> Fix the issue by moving the IOMMU type check to after the user mem maps
> traversal, thereby ensuring that by the time the check happens, the
> IOMMU type is always valid.
>
> Fixes: 56259f7fc010 ("vfio: allow partially unmapping adjacent memory")
> Cc: xuan.ding@intel.com
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>

I guess Xuan tested it too, since we have a vhost patch on top of this
vfio patch.
Can you just confirm it is ok to merge?

Thanks.
Xuan Ding Oct. 28, 2021, 6:05 a.m. UTC | #2
Hi David,

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Wednesday, October 27, 2021 10:49 PM
>To: Burakov, Anatoly <anatoly.burakov@intel.com>; Ding, Xuan
><xuan.ding@intel.com>
>Cc: dev <dev@dpdk.org>; Maxime Coquelin <maxime.coquelin@redhat.com>
>Subject: Re: [dpdk-dev] [PATCH v1 1/1] vfio: fix partial unmap check
>
>On Tue, Oct 26, 2021 at 3:30 PM Anatoly Burakov
><anatoly.burakov@intel.com> wrote:
>>
>> Partial unmap support was introduced in commit c13ca4e81cac, and with it
>> was added a check that dereferenced the IOMMU type to determine
>whether
>> partial ummapping is supported for currently configured IOMMU type. In
>> certain circumstances (such as when VFIO is supported, but no devices
>> were bound to the VFIO driver), the IOMMU type pointer can be NULL.
>>
>> However, dereferencing of IOMMU type was guarded by access to the user
>> maps list - that is, we were always checking the user map list first,
>> and then, if we found a memory region that encloses the one we're trying
>> to unmap, we would have performed the IOMMU type check.
>>
>> This ensured that the IOMMU type check will not cause any NULL pointer
>> dereferences, because in order for an IOMMU type check to have been
>> performed, there necessarily must have been at least one memory region
>> that was previously mapped successfully, and that implies having a
>> defined IOMMU type.
>>
>> When 56259f7fc010 was introduced, the IOMMU type check was moved to
>> before we were traversing the user mem maps list, thereby introducing a
>> potential NULL dereference, because the IOMMU type access was no longer
>> guarded by the user mem maps list traversal.
>>
>> Fix the issue by moving the IOMMU type check to after the user mem maps
>> traversal, thereby ensuring that by the time the check happens, the
>> IOMMU type is always valid.
>>
>> Fixes: 56259f7fc010 ("vfio: allow partially unmapping adjacent memory")
>> Cc: xuan.ding@intel.com
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>Reviewed-by: David Marchand <david.marchand@redhat.com>
>
>I guess Xuan tested it too, since we have a vhost patch on top of this
>vfio patch.
>Can you just confirm it is ok to merge?

Yes, I tested it and it works fine.

Tested-by: Xuan Ding <xuan.ding@intel.com>

Regards,
Xuan


>
>Thanks.
>
>
>--
>David Marchand
David Marchand Oct. 28, 2021, 7:52 a.m. UTC | #3
On Thu, Oct 28, 2021 at 8:06 AM Ding, Xuan <xuan.ding@intel.com> wrote:
> >> Partial unmap support was introduced in commit c13ca4e81cac, and with it
> >> was added a check that dereferenced the IOMMU type to determine
> >whether
> >> partial ummapping is supported for currently configured IOMMU type. In
> >> certain circumstances (such as when VFIO is supported, but no devices
> >> were bound to the VFIO driver), the IOMMU type pointer can be NULL.
> >>
> >> However, dereferencing of IOMMU type was guarded by access to the user
> >> maps list - that is, we were always checking the user map list first,
> >> and then, if we found a memory region that encloses the one we're trying
> >> to unmap, we would have performed the IOMMU type check.
> >>
> >> This ensured that the IOMMU type check will not cause any NULL pointer
> >> dereferences, because in order for an IOMMU type check to have been
> >> performed, there necessarily must have been at least one memory region
> >> that was previously mapped successfully, and that implies having a
> >> defined IOMMU type.
> >>
> >> When 56259f7fc010 was introduced, the IOMMU type check was moved to
> >> before we were traversing the user mem maps list, thereby introducing a
> >> potential NULL dereference, because the IOMMU type access was no longer
> >> guarded by the user mem maps list traversal.
> >>
> >> Fix the issue by moving the IOMMU type check to after the user mem maps
> >> traversal, thereby ensuring that by the time the check happens, the
> >> IOMMU type is always valid.
> >>
> >> Fixes: 56259f7fc010 ("vfio: allow partially unmapping adjacent memory")
> >> Cc: xuan.ding@intel.com
> >>
> >> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >Reviewed-by: David Marchand <david.marchand@redhat.com>
> Tested-by: Xuan Ding <xuan.ding@intel.com>

Applied, thanks.
diff mbox series

Patch

diff --git a/lib/eal/linux/eal_vfio.c b/lib/eal/linux/eal_vfio.c
index 657c89ca58..aa2087a2da 100644
--- a/lib/eal/linux/eal_vfio.c
+++ b/lib/eal/linux/eal_vfio.c
@@ -1943,9 +1943,6 @@  container_dma_unmap(struct vfio_config *vfio_cfg, uint64_t vaddr, uint64_t iova,
 	 * mappings, let's just rebuild them using information we have.
 	 */
 
-	/* do we have partial unmap capability? */
-	has_partial_unmap = vfio_cfg->vfio_iommu_type->partial_unmap;
-
 	/*
 	 * first thing to do is check if there exists a mapping that includes
 	 * the start and the end of our requested unmap. We need to collect all
@@ -1961,6 +1958,9 @@  container_dma_unmap(struct vfio_config *vfio_cfg, uint64_t vaddr, uint64_t iova,
 		goto out;
 	}
 
+	/* do we have partial unmap capability? */
+	has_partial_unmap = vfio_cfg->vfio_iommu_type->partial_unmap;
+
 	/*
 	 * if we don't support partial unmap, we must check if start and end of
 	 * current unmap region are chunk-aligned.