[PATCH 00/11] codeql fixes for various subsystems

[PATCH 00/11] codeql fixes for various subsystems

[okaya]

From: Sinan Kaya <okaya@kernel.org>

Following up the codeql reported problems first submitted
by Stephen Hemminger here:

https://lore.kernel.org/all/20220527161210.77212d0b@hermes.local/t/

Posting a series of fixes about potential null pointer accesses.

Sinan Kaya (11):
  ethdev: check return result of rte_eth_dev_info_get
  net/tap: check if name is null
  memzone: check result of rte_fbarray_get
  memzone: check result of malloc_elem_from_data
  malloc: malloc_elem_join_adjacent_free can return null
  malloc: check result of rte_mem_virt2memseg_list
  malloc: check result of rte_fbarray_get
  malloc: check result of rte_mem_virt2memseg
  malloc: check result of malloc_elem_free
  malloc: check result of elem_start_pt
  bus/vdev: check result of rte_vdev_device_name

 drivers/net/tap/rte_eth_tap.c        |  4 ++++
 lib/eal/common/eal_common_memalloc.c |  4 +++-
 lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
 lib/eal/common/malloc_elem.c         | 14 +++++++++++---
 lib/eal/common/malloc_heap.c         | 11 ++++++++++-
 lib/ethdev/ethdev_vdev.h             |  2 ++
 lib/ethdev/rte_class_eth.c           |  4 +++-
 7 files changed, 42 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 01/11] ethdev: check return result of rte_eth_dev_info_get

[okaya]

From: Sinan Kaya <okaya@kernel.org>

rte_class_eth: eth_mac_cmp: The status of this call to rte_eth_dev_info_get
is not checked, potentially leaving dev_info uninitialized.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/ethdev/rte_class_eth.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_class_eth.c b/lib/ethdev/rte_class_eth.c
index 838b3a8f9f..8165e5adc0 100644
--- a/lib/ethdev/rte_class_eth.c
+++ b/lib/ethdev/rte_class_eth.c
@@ -51,7 +51,9 @@ eth_mac_cmp(const char *key __rte_unused,
 		return -1; /* invalid devargs value */
 
 	/* Return 0 if devargs MAC is matching one of the device MACs. */
-	rte_eth_dev_info_get(data->port_id, &dev_info);
+	if (rte_eth_dev_info_get(data->port_id, &dev_info) < 0)
+		return -1;
+
 	for (index = 0; index < dev_info.max_mac_addrs; index++)
 		if (rte_is_same_ether_addr(&mac, &data->mac_addrs[index]))
 			return 0;
-- 
2.25.1

[PATCH 02/11] net/tap: check if name is null

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In rte_pmd_tun_probe result of call to rte_vdev_device_name is
dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 drivers/net/tap/rte_eth_tap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f2a6c33a19..aa640f8acc 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2340,6 +2340,10 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	struct rte_eth_dev *eth_dev;
 
 	name = rte_vdev_device_name(dev);
+	if (!name) {
+		return -1;
+	}
+
 	params = rte_vdev_device_args(dev);
 	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
 
-- 
2.25.1

[PATCH 03/11] memzone: check result of rte_fbarray_get

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In memzone_lookup_thread_unsafe result of call to rte_fbarray_get
is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/eal_common_memzone.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/eal/common/eal_common_memzone.c b/lib/eal/common/eal_common_memzone.c
index 860fb5fb64..0ed03ad337 100644
--- a/lib/eal/common/eal_common_memzone.c
+++ b/lib/eal/common/eal_common_memzone.c
@@ -41,7 +41,7 @@ memzone_lookup_thread_unsafe(const char *name)
 	i = rte_fbarray_find_next_used(arr, 0);
 	while (i >= 0) {
 		mz = rte_fbarray_get(arr, i);
-		if (mz->addr != NULL &&
+		if (mz && mz->addr != NULL &&
 				!strncmp(name, mz->name, RTE_MEMZONE_NAMESIZE))
 			return mz;
 		i = rte_fbarray_find_next_used(arr, i + 1);
@@ -358,6 +358,10 @@ dump_memzone(const struct rte_memzone *mz, void *arg)
 	fprintf(f, "physical segments used:\n");
 	ms_idx = RTE_PTR_DIFF(mz->addr, msl->base_va) / page_sz;
 	ms = rte_fbarray_get(&msl->memseg_arr, ms_idx);
+	if (!ms) {
+		RTE_LOG(DEBUG, EAL, "Skipping bad memzone\n");
+		return;
+	}
 
 	do {
 		fprintf(f, "  addr: %p iova: 0x%" PRIx64 " "
-- 
2.25.1

[PATCH 04/11] memzone: check result of malloc_elem_from_data

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In memzone_reserve_aligned_thread_unsafe result of call
to malloc_elem_from_data is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/eal_common_memzone.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/eal/common/eal_common_memzone.c b/lib/eal/common/eal_common_memzone.c
index 0ed03ad337..74aa5ac114 100644
--- a/lib/eal/common/eal_common_memzone.c
+++ b/lib/eal/common/eal_common_memzone.c
@@ -169,6 +169,10 @@ memzone_reserve_aligned_thread_unsafe(const char *name, size_t len,
 	}
 
 	struct malloc_elem *elem = malloc_elem_from_data(mz_addr);
+	if (!elem) {
+		rte_errno = ENOSPC;
+		return NULL;
+	}
 
 	/* fill the zone in config */
 	mz_idx = rte_fbarray_find_next_free(arr, 0);
-- 
2.25.1

[PATCH 05/11] malloc: malloc_elem_join_adjacent_free can return null

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In malloc_heap_add_memory result of call to malloc_elem_join_adjacent_free
is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/malloc_heap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index d7c410b786..d2ccc387bf 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -97,6 +97,9 @@ malloc_heap_add_memory(struct malloc_heap *heap, struct rte_memseg_list *msl,
 	malloc_elem_insert(elem);
 
 	elem = malloc_elem_join_adjacent_free(elem);
+	if (!elem) {
+		return NULL;
+	}
 
 	malloc_elem_free_list_insert(elem);
 
-- 
2.25.1

[PATCH 06/11] malloc: check result of rte_mem_virt2memseg_list

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In alloc_pages_on_heap result of call to rte_mem_virt2memseg_list
is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/malloc_heap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index d2ccc387bf..438c0856e2 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -324,6 +324,9 @@ alloc_pages_on_heap(struct malloc_heap *heap, uint64_t pg_sz, size_t elt_size,
 
 	map_addr = ms[0]->addr;
 	msl = rte_mem_virt2memseg_list(map_addr);
+	if (!msl) {
+		return NULL;
+	}
 
 	/* check if we wanted contiguous memory but didn't get it */
 	if (contig && !eal_memalloc_is_contig(msl, map_addr, alloc_sz)) {
-- 
2.25.1

[PATCH 07/11] malloc: check result of rte_fbarray_get

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In eal_memalloc_is_contig result of call to rte_fbarray_get
is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/eal_common_memalloc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/eal/common/eal_common_memalloc.c b/lib/eal/common/eal_common_memalloc.c
index ab04479c1c..e7f4bede39 100644
--- a/lib/eal/common/eal_common_memalloc.c
+++ b/lib/eal/common/eal_common_memalloc.c
@@ -126,6 +126,8 @@ eal_memalloc_is_contig(const struct rte_memseg_list *msl, void *start,
 
 		/* skip first iteration */
 		ms = rte_fbarray_get(&msl->memseg_arr, start_seg);
+		if (!ms)
+			return false;
 		cur = ms->iova;
 		expected = cur + pgsz;
 
@@ -137,7 +139,7 @@ eal_memalloc_is_contig(const struct rte_memseg_list *msl, void *start,
 				cur_seg++, expected += pgsz) {
 			ms = rte_fbarray_get(&msl->memseg_arr, cur_seg);
 
-			if (ms->iova != expected)
+			if (ms && (ms->iova != expected))
 				return false;
 		}
 	}
-- 
2.25.1

[PATCH 08/11] malloc: check result of rte_mem_virt2memseg

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In malloc_elem_find_max_iova_contig result of call to rte_mem_virt2memseg
is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/malloc_elem.c | 11 ++++++++---
 lib/eal/common/malloc_heap.c |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/eal/common/malloc_elem.c b/lib/eal/common/malloc_elem.c
index 83f05497cc..54d7b2f278 100644
--- a/lib/eal/common/malloc_elem.c
+++ b/lib/eal/common/malloc_elem.c
@@ -63,6 +63,8 @@ malloc_elem_find_max_iova_contig(struct malloc_elem *elem, size_t align)
 
 	cur_page = RTE_PTR_ALIGN_FLOOR(contig_seg_start, page_sz);
 	ms = rte_mem_virt2memseg(cur_page, elem->msl);
+	if (!ms)
+		return 0;
 
 	/* do first iteration outside the loop */
 	page_end = RTE_PTR_ADD(cur_page, page_sz);
@@ -91,9 +93,12 @@ malloc_elem_find_max_iova_contig(struct malloc_elem *elem, size_t align)
 			 * we're not blowing past data end.
 			 */
 			ms = rte_mem_virt2memseg(contig_seg_start, elem->msl);
-			cur_page = ms->addr;
-			/* don't trigger another recalculation */
-			expected_iova = ms->iova;
+			if (ms) {
+				cur_page = ms->addr;
+
+				/* don't trigger another recalculation */
+				expected_iova = ms->iova;
+			}
 			continue;
 		}
 		/* cur_seg_end ends on a page boundary or on data end. if we're
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index 438c0856e2..1bf2e94c83 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -932,7 +932,7 @@ malloc_heap_free(struct malloc_elem *elem)
 		const struct rte_memseg *tmp =
 				rte_mem_virt2memseg(aligned_start, msl);
 
-		if (tmp->flags & RTE_MEMSEG_FLAG_DO_NOT_FREE) {
+		if (tmp && (tmp->flags & RTE_MEMSEG_FLAG_DO_NOT_FREE)) {
 			/* this is an unfreeable segment, so move start */
 			aligned_start = RTE_PTR_ADD(tmp->addr, tmp->len);
 		}
-- 
2.25.1

[PATCH 09/11] malloc: check result of malloc_elem_free

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In malloc_heap_free result of call to malloc_elem_free is dereferenced
here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/malloc_heap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index 1bf2e94c83..78a540c860 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -894,6 +894,9 @@ malloc_heap_free(struct malloc_elem *elem)
 	/* anything after this is a bonus */
 	ret = 0;
 
+	if (!elem)
+		goto free_unlock;
+
 	/* ...of which we can't avail if we are in legacy mode, or if this is an
 	 * externally allocated segment.
 	 */
-- 
2.25.1

Re: [PATCH 02/11] net/tap: check if name is null

[Thomas Monjalon]

21/11/2022 21:40, okaya@kernel.org:
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -2340,6 +2340,10 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
>  	struct rte_eth_dev *eth_dev;
>  
>  	name = rte_vdev_device_name(dev);
> +	if (!name) {

Please it is preferred to check against NULL,
because name is not a boolean, thanks.
I know it's longer but it is more explicit.

Thanks for the fixes in this series.

Re: [PATCH 02/11] net/tap: check if name is null

[Sinan Kaya]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

On Mon, 2022-11-21 at 22:41 +0100, Thomas Monjalon wrote:
> 21/11/2022 21:40, okaya@kernel.org:
> > --- a/drivers/net/tap/rte_eth_tap.c+++
> > b/drivers/net/tap/rte_eth_tap.c@@ -2340,6 +2340,10 @@
> > rte_pmd_tun_probe(struct rte_vdev_device *dev) 	struct
> > rte_eth_dev *eth_dev;  	name = rte_vdev_device_name(dev);+	if
> > (!name) {
> 
> Please it is preferred to check against NULL,because name is not a
> boolean, thanks.I know it's longer but it is more explicit.

Sure, I can do that. Getting used to dpdk coding style. I wasn't sure
what to do with braces on single line too. At least, I got a warning on
that too.

> Thanks for the fixes in this series.
> 

Cheers

[-- Attachment #2: Type: text/html, Size: 1468 bytes --]

Re: [PATCH 02/11] net/tap: check if name is null

[Ferruh Yigit]

On 11/21/2022 10:03 PM, Sinan Kaya wrote:
> On Mon, 2022-11-21 at 22:41 +0100, Thomas Monjalon wrote:
>> 21/11/2022 21:40, 
>> okaya@kernel.org
>>  <mailto:okaya@kernel.org>
>> :
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -2340,6 +2340,10 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
>>>  	struct rte_eth_dev *eth_dev;
>>>  
>>>  	name = rte_vdev_device_name(dev);
>>> +	if (!name) {
>>
>> Please it is preferred to check against NULL,
>> because name is not a boolean, thanks.
>> I know it's longer but it is more explicit.
> 
> Sure, I can do that. Getting used to dpdk coding style. I wasn't sure
> what to do with braces on single line too. At least, I got a warning on
> that too.
> 

DPDK coding convention is documented if it helps:
https://doc.dpdk.org/guides/contributing/coding_style.html

>>
>> Thanks for the fixes in this series.
>>
>>
> 
> Cheers
>