diff mbox series

[RFC,1/5] bus/pci: fix allocation of pci device path

Message ID 20181106214901.1392-2-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series more Coverity related bug fixes |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger Nov. 6, 2018, 9:48 p.m. UTC 
  The pci_resource_by_index called strlen() on uninitialized
memory which would lead to the wrong size of memory allocated
for the path portion of the resource map. This would either cause
excessively large allocation, or worse memory corruption.

Coverity Issue: 300868
Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/bus/pci/linux/pci_uio.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

Comments

Thomas Monjalon Nov. 18, 2018, 3:03 p.m. UTC | #1 
06/11/2018 22:48, Stephen Hemminger:
> The pci_resource_by_index called strlen() on uninitialized
> memory which would lead to the wrong size of memory allocated
> for the path portion of the resource map. This would either cause
> excessively large allocation, or worse memory corruption.
> 
> Coverity Issue: 300868
> Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

This patch is RFC but Ferruh (UIO maintainer) was not Cc'ed.
I feel this bug is critical. Please advise.
Ferruh Yigit Nov. 22, 2018, 11:52 p.m. UTC | #2 
On 11/6/2018 9:48 PM, Stephen Hemminger wrote:
> The pci_resource_by_index called strlen() on uninitialized
> memory which would lead to the wrong size of memory allocated
> for the path portion of the resource map. This would either cause
> excessively large allocation, or worse memory corruption.

Yes this may corrupt memory, I wonder how nobody hit this. I am for including
the fix for the release.

> 
> Coverity Issue: 300868
> Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/bus/pci/linux/pci_uio.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index a7c14421aa79..112ac51dddcc 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -295,14 +295,6 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>  	loc = &dev->addr;
>  	maps = uio_res->maps;
>  
> -	/* allocate memory to keep path */
> -	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
> -	if (maps[map_idx].path == NULL) {
> -		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
> -				strerror(errno));
> -		return -1;
> -	}

What about simply:

-       maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
+       maps[map_idx].path = rte_malloc(NULL, sizeof(devname) + 1, 0);
diff mbox series

Patch

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index a7c14421aa79..112ac51dddcc 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -295,14 +295,6 @@  pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	loc = &dev->addr;
 	maps = uio_res->maps;
 
-	/* allocate memory to keep path */
-	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
-	if (maps[map_idx].path == NULL) {
-		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
-				strerror(errno));
-		return -1;
-	}
-
 	/*
 	 * open resource file, to mmap it
 	 */
@@ -335,10 +327,19 @@  pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 		if (fd < 0) {
 			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 				devname, strerror(errno));
-			goto error;
+			return -1;
 		}
 	}
 
+	/* allocate memory to keep path */
+	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
+	if (maps[map_idx].path == NULL) {
+		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
+				strerror(errno));
+		close(fd);
+		return -1;
+	}
+
 	/* try mapping somewhere close to the end of hugepages */
 	if (pci_map_addr == NULL)
 		pci_map_addr = pci_find_max_end_va();
@@ -346,8 +347,10 @@  pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	mapaddr = pci_map_resource(pci_map_addr, fd, 0,
 			(size_t)dev->mem_resource[res_idx].len, 0);
 	close(fd);
-	if (mapaddr == MAP_FAILED)
-		goto error;
+	if (mapaddr == MAP_FAILED) {
+		rte_free(maps[map_idx].path);
+		return -1;
+	}
 
 	pci_map_addr = RTE_PTR_ADD(mapaddr,
 			(size_t)dev->mem_resource[res_idx].len);
@@ -360,10 +363,6 @@  pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	dev->mem_resource[res_idx].addr = mapaddr;
 
 	return 0;
-
-error:
-	rte_free(maps[map_idx].path);
-	return -1;
 }
 
 #if defined(RTE_ARCH_X86)