* [bug report] vfio: hugepage support for vfio_iommu_type1
@ 2017-10-12 12:40 Dan Carpenter
2017-10-12 16:32 ` Alex Williamson
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2017-10-12 12:40 UTC (permalink / raw)
To: alex.williamson; +Cc: kvm
Hello Alex Williamson,
The patch 166fd7d94afd: "vfio: hugepage support for vfio_iommu_type1"
from Jun 21, 2013, leads to the following static checker warning:
drivers/vfio/vfio_iommu_type1.c:819 vfio_dma_do_unmap()
warn: overflowed symbol reused: 'unmap->size'
drivers/vfio/vfio_iommu_type1.c
756 static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
757 struct vfio_iommu_type1_dma_unmap *unmap)
758 {
759 uint64_t mask;
760 struct vfio_dma *dma, *dma_last = NULL;
761 size_t unmapped = 0;
762 int ret = 0, retries = 0;
763
764 mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
765
766 if (unmap->iova & mask)
767 return -EINVAL;
768 if (!unmap->size || unmap->size & mask)
769 return -EINVAL;
unmap is controlled by the user from the ioctl. ->iova and ->size are
type u64. It would be simple enough to add an integer overflow check
here, but I'd be tempted to do something like:
if (unmap->size + unmap->iova < unmap->size ||
unmap->size + unmap->iova > SIZE_MAX)
return -EINVAL;
Because below we are passing it to vfio_find_dma() which only takes a
size_t and not a u64.
I don't think it actually matters? The heuristic here is that we look
for user triggered integer overflows and then we re-use the variables
again. There are too many integer overflows (thousands) which aren't
harmful and I haven't yet figured out an easy way to filter for just
the harmful ones...
770
771 WARN_ON(mask & PAGE_MASK);
772 again:
773 mutex_lock(&iommu->lock);
774
775 /*
776 * vfio-iommu-type1 (v1) - User mappings were coalesced together to
777 * avoid tracking individual mappings. This means that the granularity
778 * of the original mapping was lost and the user was allowed to attempt
779 * to unmap any range. Depending on the contiguousness of physical
780 * memory and page sizes supported by the IOMMU, arbitrary unmaps may
781 * or may not have worked. We only guaranteed unmap granularity
782 * matching the original mapping; even though it was untracked here,
783 * the original mappings are reflected in IOMMU mappings. This
784 * resulted in a couple unusual behaviors. First, if a range is not
785 * able to be unmapped, ex. a set of 4k pages that was mapped as a
786 * 2M hugepage into the IOMMU, the unmap ioctl returns success but with
787 * a zero sized unmap. Also, if an unmap request overlaps the first
788 * address of a hugepage, the IOMMU will unmap the entire hugepage.
789 * This also returns success and the returned unmap size reflects the
790 * actual size unmapped.
791 *
792 * We attempt to maintain compatibility with this "v1" interface, but
793 * we take control out of the hands of the IOMMU. Therefore, an unmap
794 * request offset from the beginning of the original mapping will
795 * return success with zero sized unmap. And an unmap request covering
796 * the first iova of mapping will unmap the entire range.
797 *
798 * The v2 version of this interface intends to be more deterministic.
799 * Unmap requests must fully cover previous mappings. Multiple
800 * mappings may still be unmaped by specifying large ranges, but there
801 * must not be any previous mappings bisected by the range. An error
802 * will be returned if these conditions are not met. The v2 interface
803 * will only return success and a size of zero if there were no
804 * mappings within the range.
805 */
806 if (iommu->v2) {
807 dma = vfio_find_dma(iommu, unmap->iova, 1);
808 if (dma && dma->iova != unmap->iova) {
809 ret = -EINVAL;
810 goto unlock;
811 }
812 dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
813 if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
814 ret = -EINVAL;
815 goto unlock;
816 }
817 }
818
819 while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
820 if (!iommu->v2 && unmap->iova > dma->iova)
821 break;
822 /*
823 * Task with same address space who mapped this iova range is
824 * allowed to unmap the iova range.
825 */
826 if (dma->task->mm != current->mm)
827 break;
828
829 if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
830 struct vfio_iommu_type1_dma_unmap nb_unmap;
831
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] vfio: hugepage support for vfio_iommu_type1
2017-10-12 12:40 [bug report] vfio: hugepage support for vfio_iommu_type1 Dan Carpenter
@ 2017-10-12 16:32 ` Alex Williamson
2017-10-12 16:54 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Alex Williamson @ 2017-10-12 16:32 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kvm
Hi Dan,
On Thu, 12 Oct 2017 15:40:36 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Hello Alex Williamson,
>
> The patch 166fd7d94afd: "vfio: hugepage support for vfio_iommu_type1"
> from Jun 21, 2013, leads to the following static checker warning:
>
> drivers/vfio/vfio_iommu_type1.c:819 vfio_dma_do_unmap()
> warn: overflowed symbol reused: 'unmap->size'
>
> drivers/vfio/vfio_iommu_type1.c
> 756 static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> 757 struct vfio_iommu_type1_dma_unmap *unmap)
> 758 {
> 759 uint64_t mask;
> 760 struct vfio_dma *dma, *dma_last = NULL;
> 761 size_t unmapped = 0;
> 762 int ret = 0, retries = 0;
> 763
> 764 mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
> 765
> 766 if (unmap->iova & mask)
> 767 return -EINVAL;
> 768 if (!unmap->size || unmap->size & mask)
> 769 return -EINVAL;
>
> unmap is controlled by the user from the ioctl. ->iova and ->size are
> type u64. It would be simple enough to add an integer overflow check
> here, but I'd be tempted to do something like:
>
> if (unmap->size + unmap->iova < unmap->size ||
> unmap->size + unmap->iova > SIZE_MAX)
> return -EINVAL;
>
> Because below we are passing it to vfio_find_dma() which only takes a
> size_t and not a u64.
Ok, but why test unmap->iova + unmap->size relative to SIZE_MAX rather
than just unmap->size since we use a dma_addr_t for the iova in
vfio_find_dma()?
> I don't think it actually matters? The heuristic here is that we look
> for user triggered integer overflows and then we re-use the variables
> again. There are too many integer overflows (thousands) which aren't
> harmful and I haven't yet figured out an easy way to filter for just
> the harmful ones...
Right, overflows here seem like they could only hurt the user in
management of their own iova address space, not the kernel integrity,
but if adding the above check provides a sanity check for the user and
makes debugging a little easier, I'm for it. Thanks,
Alex
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] vfio: hugepage support for vfio_iommu_type1
2017-10-12 16:32 ` Alex Williamson
@ 2017-10-12 16:54 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2017-10-12 16:54 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm
On Thu, Oct 12, 2017 at 10:32:41AM -0600, Alex Williamson wrote:
> Hi Dan,
>
> On Thu, 12 Oct 2017 15:40:36 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> > Hello Alex Williamson,
> >
> > The patch 166fd7d94afd: "vfio: hugepage support for vfio_iommu_type1"
> > from Jun 21, 2013, leads to the following static checker warning:
> >
> > drivers/vfio/vfio_iommu_type1.c:819 vfio_dma_do_unmap()
> > warn: overflowed symbol reused: 'unmap->size'
> >
> > drivers/vfio/vfio_iommu_type1.c
> > 756 static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> > 757 struct vfio_iommu_type1_dma_unmap *unmap)
> > 758 {
> > 759 uint64_t mask;
> > 760 struct vfio_dma *dma, *dma_last = NULL;
> > 761 size_t unmapped = 0;
> > 762 int ret = 0, retries = 0;
> > 763
> > 764 mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
> > 765
> > 766 if (unmap->iova & mask)
> > 767 return -EINVAL;
> > 768 if (!unmap->size || unmap->size & mask)
> > 769 return -EINVAL;
> >
> > unmap is controlled by the user from the ioctl. ->iova and ->size are
> > type u64. It would be simple enough to add an integer overflow check
> > here, but I'd be tempted to do something like:
> >
> > if (unmap->size + unmap->iova < unmap->size ||
> > unmap->size + unmap->iova > SIZE_MAX)
> > return -EINVAL;
> >
> > Because below we are passing it to vfio_find_dma() which only takes a
> > size_t and not a u64.
>
> Ok, but why test unmap->iova + unmap->size relative to SIZE_MAX rather
> than just unmap->size since we use a dma_addr_t for the iova in
> vfio_find_dma()?
Oh, yeah, you're right. I misread the code a bit. I'll send a check
for that then.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-12 16:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-12 12:40 [bug report] vfio: hugepage support for vfio_iommu_type1 Dan Carpenter
2017-10-12 16:32 ` Alex Williamson
2017-10-12 16:54 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox