vfio: Fix coverity found issues
Subsystems (2)
cover vfio: Fix coverity found issues
Overall nothing very serious here. We botch the size of a PCIe capability due to a cut-n-paste error, but nobody has noticed due to the rarity of the capability. A 32bit host could possibly see some overflows attempting to do a DMA map, but all of the parameters are also validated elsewhere, so while this may cause unexepected behavior for the user, there doesn't appear to be any risk to the host. Thanks,
Alex
---
Alex Williamson (3):
vfio/pci: Fix sizing of DPA and THP express capabilities vfio/pci: Fix unchecked return value vfio/iommu_type1: Avoid overflow
drivers/vfio/pci/vfio_pci.c | 3 ++ drivers/vfio/pci/vfio_pci_config.c | 7 ++---- drivers/vfio/vfio_iommu_type1.c | 45 ++++++++++++++---------------------- 3 files changed, 23 insertions(+), 32 deletions(-)
PATCH 1/3 vfio/pci: Fix sizing of DPA and THP express capabilities
When sizing the TPH capability we store the register containing the table size into the 'dword' variable, but then use the uninitialized 'byte' variable to analyze the size. The table size is also actually reported as an N-1 value, so correct sizing to account for this.
The round_up() for both TPH and DPA is unnecessary, remove it.
Detected by Coverity: CID 714665 & 715156
--- drivers/vfio/pci/vfio_pci_config.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/vfio/pci/vfio_pci_config.c +3 -4
@@ -1126,8 +1126,7 @@ static int vfio_ext_cap_len(struct vfio_pci_device *vdev, u16 ecap, u16 epos)return pcibios_err_to_errno (ret); byte &=PCI_DPA_CAP_SUBSTATE_MASK ; - byte =round_up (byte +1 ,4 ); -return PCI_DPA_BASE_SIZEOF + byte; +return PCI_DPA_BASE_SIZEOF + byte +1 ;case PCI_EXT_CAP_ID_TPH : ret =pci_read_config_dword (pdev, epos +PCI_TPH_CAP , &dword);if (ret)
@@ -1136,9 +1135,9 @@ static int vfio_ext_cap_len(struct vfio_pci_device *vdev, u16 ecap, u16 epos)if ((dword &PCI_TPH_CAP_LOC_MASK ) ==PCI_TPH_LOC_CAP ) {int sts; - sts = byte &PCI_TPH_CAP_ST_MASK ; + sts = dword &PCI_TPH_CAP_ST_MASK ; sts >>=PCI_TPH_CAP_ST_SHIFT ; -return PCI_TPH_BASE_SIZEOF +round_up (sts *2 ,4 ); +return PCI_TPH_BASE_SIZEOF + (sts *2 ) +2 ; }return PCI_TPH_BASE_SIZEOF ;default :
PATCH 2/3 vfio/pci: Fix unchecked return value
In reply to Alex WilliamsonThere's nothing we can do different if pci_load_and_free_saved_state() fails, other than maybe print some log message, but the actual re-load of the state is an unnecessary step here since we've only just saved it. We can cleanup a coverity warning and eliminate the unnecessary stop by freeing the state ourselves.
Detected by Coverity: CID 753101
--- drivers/vfio/pci/vfio_pci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/vfio/pci/vfio_pci.c +2 -1
@@ -57,7 +57,8 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) ret =vfio_config_init (vdev);if (ret) { -pci_load_and_free_saved_state (pdev, &vdev->pci_saved_state); +kfree (vdev->pci_saved_state); + vdev->pci_saved_state =NULL ;pci_disable_device (pdev);return ret; }
PATCH 3/3 vfio/iommu_type1: Avoid overflow
In reply to Alex WilliamsonCoverity reports use of a tained scalar used as a loop boundary. For the most part, any values passed from userspace for a DMA mapping size, IOVA, or virtual address are valid, with some alignment constraints. The size is ultimately bound by how many pages the user is able to lock, IOVA is tested by the IOMMU driver when doing a map, and the virtual address needs to pass get_user_pages. The only problem I can find is that we do expect the __u64 user values to fit within our variables, which might not happen on 32bit platforms. Add a test for this and return error on overflow. Also propagate use of the type-correct local variables throughout the function.
The above also points to the 'end' variable, which can be zero if we're operating at the very top of the address space. We try to account for this, but our loop botches it. Rework the loop to use the remaining size as our loop condition rather than the IOVA vs end.
Detected by Coverity: CID 714659
--- drivers/vfio/vfio_iommu_type1.c | 45 ++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 27 deletions(-)
drivers/vfio/vfio_iommu_type1.c +18 -27
@@ -524,7 +524,7 @@ unwind:static int vfio_dma_do_map (struct vfio_iommu *iommu,struct vfio_iommu_type1_dma_map *map) { -dma_addr_t end, iova; +dma_addr_t iova = map->iova;unsigned long vaddr = map->vaddr;size_t size = map->size;long npage;
@@ -533,39 +533,30 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,struct vfio_dma *dma;unsigned long pfn; - end = map->iova + map->size; +/* Verify that none of our __u64 fields overflow */ +if (map->size != size || map->vaddr != vaddr || map->iova != iova) +return -EINVAL ; mask = ((uint64_t )1 <<__ffs (vfio_pgsize_bitmap (iommu))) -1 ; +WARN_ON (mask &PAGE_MASK ); +/* READ/WRITE from device perspective */ if (map->flags &VFIO_DMA_MAP_FLAG_WRITE ) prot |=IOMMU_WRITE ;if (map->flags &VFIO_DMA_MAP_FLAG_READ ) prot |=IOMMU_READ ; -if (!prot) -return -EINVAL ;/* No READ/WRITE? */ - -if (vaddr & mask) -return -EINVAL ; -if (map->iova & mask) -return -EINVAL ; -if (!map->size || map->size & mask) -return -EINVAL ; - -WARN_ON (mask &PAGE_MASK ); - -/* Don't allow IOVA wrap */ -if (end && end < map->iova) +if (!prot || !size || (size | iova | vaddr) & mask)return -EINVAL ; -/* Don't allow virtual address wrap */ -if (vaddr + map->size && vaddr + map->size < vaddr) +/* Don't allow IOVA or virtual address wrap */ +if (iova + size -1 < iova || vaddr + size -1 < vaddr)return -EINVAL ;mutex_lock (&iommu->lock); -if (vfio_find_dma (iommu, map->iova, map->size)) { +if (vfio_find_dma (iommu, iova, size)) {mutex_unlock (&iommu->lock);return -EEXIST ; }
@@ -576,17 +567,17 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,return -ENOMEM ; } - dma->iova = map->iova; - dma->vaddr = map->vaddr; + dma->iova = iova; + dma->vaddr = vaddr; dma->prot = prot;/* Insert zero-sized and grow as we map chunks of it */ vfio_link_dma (iommu, dma); -for (iova = map->iova; iova < end; iova += size, vaddr += size) { +while (size) {/* Pin a contiguous chunk of memory */ - npage =vfio_pin_pages (vaddr, (end - iova) >>PAGE_SHIFT , - prot, &pfn); + npage =vfio_pin_pages (vaddr + dma->size, + size >>PAGE_SHIFT , prot, &pfn);if (npage <=0 ) {WARN_ON (!npage); ret = (int )npage;
@@ -594,14 +585,14 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu, }/* Map it! */ - ret =vfio_iommu_map (iommu, iova, pfn, npage, prot); + ret =vfio_iommu_map (iommu, iova + dma->size, pfn, npage, prot);if (ret) {vfio_unpin_pages (pfn, npage, prot, true);break ; } - size = npage <<PAGE_SHIFT ; - dma->size += size; + size -= npage <<PAGE_SHIFT ; + dma->size += npage <<PAGE_SHIFT ; }if (ret)