codeql fixes for various subsystems

PATCH In review [email protected] 7 participants 29 messages Jan 20, 2023 – Dec 16, 2023

lore

Share & tools

lore

[email protected]

author

2023-01-20 04:41 UTC #0 lore raw

v3 cover codeql fixes for various subsystems

From: Sinan Kaya <[email protected]>

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

https://lore.kernel.org/all/[email protected]/t/

Posting a series of fixes about potential null pointer accesses.

Changes from v3:
- Dropped net/tap: check if name is null
- Moved the comment to the appropriate position in ethdev: check return result of rte_eth_dev_info_get

Changes from v2:
- Remove braces around single line statements
- use NULL comparisons

Sinan Kaya (10):
ethdev: check return result of rte_eth_dev_info_get
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

 lib/eal/common/eal_common_memalloc.c |  5 ++++-
 lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
 lib/eal/common/malloc_elem.c         | 14 +++++++++++---
 lib/eal/common/malloc_heap.c         |  9 ++++++++-
 lib/ethdev/ethdev_vdev.h             |  2 ++
 lib/ethdev/rte_class_eth.c           |  4 +++-
 6 files changed, 37 insertions(+), 7 deletions(-)

Signature · 1 line

2.25.1

[email protected]

author

2023-01-20 04:42 UTC #1 lore raw

PATCH v3 1/10 ethdev: check return result of rte_eth_dev_info_get

From: Sinan Kaya <[email protected]>

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 <[email protected]>

---
 lib/ethdev/rte_class_eth.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

lib/ethdev/rte_class_eth.c +3 -1

index 838b3a8f9f..559ce22491 100644
@@ -50,8 +50,10 @@ eth_mac_cmp(const char *key __rte_unused,
 	if (rte_ether_unformat_addr(value, &mac) < 0)
 		return -1; /* invalid devargs value */
 
+	if (rte_eth_dev_info_get(data->port_id, &dev_info) < 0)
+		return -1;
+
 	/* Return 0 if devargs MAC is matching one of the device MACs. */
-	rte_eth_dev_info_get(data->port_id, &dev_info);
 	for (index = 0; index < dev_info.max_mac_addrs; index++)
 		if (rte_is_same_ether_addr(&mac, &data->mac_addrs[index]))
 			return 0;

Signature · 1 line

2.25.1

Morten Brørup

Reviewed-by on v3 1/10

2023-01-20 10:21 UTC #2 lore raw

RE: [PATCH v3 01/10] ethdev: check return result of rte_eth_dev_info_get

Quoted 31 lines

From: [email protected] [mailto:[email protected]]
Sent: Friday, 20 January 2023 05.42
From: Sinan Kaya <[email protected]>
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 <[email protected]>
---
 lib/ethdev/rte_class_eth.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
lib/ethdev/rte_class_eth.c +3 -2
index 838b3a8f9f..559ce22491 100644
@@ -50,8 +50,10 @@ eth_mac_cmp(const char *key __rte_unused,
 	if (rte_ether_unformat_addr(value, &mac) < 0)
 		return -1; /* invalid devargs value */
 
+	if (rte_eth_dev_info_get(data->port_id, &dev_info) < 0)
+		return -1;
+
 	/* Return 0 if devargs MAC is matching one of the device MACs. */
-	rte_eth_dev_info_get(data->port_id, &dev_info);
 	for (index = 0; index < dev_info.max_mac_addrs; index++)
 		if (rte_is_same_ether_addr(&mac, &data->mac_addrs[index]))
 			return 0;
--

Quoted 2 lines

2.25.1

Although rte_eth_dev_info_get() can return various error codes, eth_mac_cmp() returns only 0 or -1, so this is correct.

Reviewed-by Morten Brørup <[email protected]>

[email protected]

author

2023-01-20 04:41 UTC #3 lore raw

PATCH v3 2/10 memzone: check result of rte_fbarray_get

In reply to [email protected]

From: Sinan Kaya <[email protected]>

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

Signed-off-by Sinan Kaya <[email protected]>

---
 lib/eal/common/eal_common_memzone.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

lib/eal/common/eal_common_memzone.c +5 -1

index 860fb5fb64..8d472505eb 100644
@@ -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 != NULL) && (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 == NULL) {
+		RTE_LOG(DEBUG, EAL, "Skipping bad memzone\n");
+		return;
+	}
 
 	do {
 		fprintf(f, "  addr: %p iova: 0x%" PRIx64 " "

Signature · 1 line

2.25.1

[email protected]

author

2023-01-20 04:42 UTC #4 lore raw

PATCH v3 3/10 memzone: check result of malloc_elem_from_data

In reply to [email protected]

From: Sinan Kaya <[email protected]>

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 <[email protected]>

---
 lib/eal/common/eal_common_memzone.c | 4 ++++
 1 file changed, 4 insertions(+)

lib/eal/common/eal_common_memzone.c +4 -0

index 8d472505eb..930fee5fdc 100644
@@ -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 == NULL) {
+		rte_errno = ENOSPC;
+		return NULL;
+	}
 
 	/* fill the zone in config */
 	mz_idx = rte_fbarray_find_next_free(arr, 0);

Signature · 1 line

2.25.1

[email protected]

author

2023-01-20 04:42 UTC #5 lore raw

PATCH v3 4/10 malloc: malloc_elem_join_adjacent_free can return null

In reply to [email protected]

From: Sinan Kaya <[email protected]>

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 <[email protected]>

---
 lib/eal/common/malloc_heap.c | 2 ++
 1 file changed, 2 insertions(+)

lib/eal/common/malloc_heap.c +2 -0

index d7c410b786..503e551bf9 100644
@@ -97,6 +97,8 @@ 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 == NULL)
+		return NULL;
 
 	malloc_elem_free_list_insert(elem);

Signature · 1 line

2.25.1

[email protected]

author

2023-01-20 04:42 UTC #6 lore raw

PATCH v3 5/10 malloc: check result of rte_mem_virt2memseg_list

In reply to [email protected]

From: Sinan Kaya <[email protected]>

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 <[email protected]>

---
 lib/eal/common/malloc_heap.c | 2 ++
 1 file changed, 2 insertions(+)

lib/eal/common/malloc_heap.c +2 -0

index 503e551bf9..3f41430e42 100644
@@ -323,6 +323,8 @@ 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 == NULL)
+		return NULL;
 
 	/* check if we wanted contiguous memory but didn't get it */
 	if (contig && !eal_memalloc_is_contig(msl, map_addr, alloc_sz)) {

Signature · 1 line

2.25.1

[email protected]

author

2023-01-20 04:42 UTC #7 lore raw

PATCH v3 6/10 malloc: check result of rte_fbarray_get

In reply to [email protected]

From: Sinan Kaya <[email protected]>

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

Signed-off-by Sinan Kaya <[email protected]>

---
 lib/eal/common/eal_common_memalloc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

lib/eal/common/eal_common_memalloc.c +4 -1

index ab04479c1c..24506f8447 100644
@@ -126,6 +126,9 @@ 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 == NULL)
+			return false;
+
 		cur = ms->iova;
 		expected = cur + pgsz;

@@ -137,7 +140,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 != NULL) && (ms->iova != expected))
 				return false;
 		}
 	}

Signature · 1 line

2.25.1

[email protected]

author

2023-01-20 04:42 UTC #8 lore raw

PATCH v3 7/10 malloc: check result of rte_mem_virt2memseg

In reply to [email protected]

From: Sinan Kaya <[email protected]>

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 <[email protected]>

---
 lib/eal/common/malloc_elem.c | 11 ++++++++---
 lib/eal/common/malloc_heap.c |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

lib/eal/common/malloc_elem.c +8 -3

index 83f05497cc..8f49812846 100644
@@ -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 == NULL)
+		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 != NULL) {
+				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

lib/eal/common/malloc_heap.c +1 -1

index 3f41430e42..88270ce4d2 100644
@@ -930,7 +930,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 != NULL) && (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);
 		}

Signature · 1 line

2.25.1

[email protected]

author

2023-01-20 04:42 UTC #9 lore raw

PATCH v3 8/10 malloc: check result of malloc_elem_free

In reply to [email protected]

From: Sinan Kaya <[email protected]>

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

Signed-off-by Sinan Kaya <[email protected]>

---
 lib/eal/common/malloc_heap.c | 3 +++
 1 file changed, 3 insertions(+)

lib/eal/common/malloc_heap.c +3 -0

index 88270ce4d2..6eb6fcda5e 100644
@@ -892,6 +892,9 @@ malloc_heap_free(struct malloc_elem *elem)
 	/* anything after this is a bonus */
 	ret = 0;
 
+	if (elem == NULL)
+		goto free_unlock;
+
 	/* ...of which we can't avail if we are in legacy mode, or if this is an
 	 * externally allocated segment.
 	 */

Signature · 1 line

2.25.1

[email protected]

author

2023-01-20 04:42 UTC #10 lore raw

PATCH v3 9/10 malloc: check result of elem_start_pt

In reply to [email protected]

From: Sinan Kaya <[email protected]>

In malloc_elem_alloc result of call to elem_start_pt is dereferenced here and may be null.

Signed-off-by Sinan Kaya <[email protected]>

---
 lib/eal/common/malloc_elem.c | 3 +++
 1 file changed, 3 insertions(+)

lib/eal/common/malloc_elem.c +3 -0

index 8f49812846..26296f2dba 100644
@@ -435,6 +435,9 @@ malloc_elem_alloc(struct malloc_elem *elem, size_t size, unsigned align,
 {
 	struct malloc_elem *new_elem = elem_start_pt(elem, size, align, bound,
 			contig);
+	if (new_elem == NULL)
+		return NULL;
+
 	const size_t old_elem_size = (uintptr_t)new_elem - (uintptr_t)elem;
 	const size_t trailer_size = elem->size - old_elem_size - size -
 		MALLOC_ELEM_OVERHEAD;

Signature · 1 line

2.25.1

[email protected]

author

2023-01-20 04:42 UTC #11 lore raw

PATCH v3 10/10 bus/vdev: check result of rte_vdev_device_name

In reply to [email protected]

From: Sinan Kaya <[email protected]>

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

Signed-off-by Sinan Kaya <[email protected]>

---
 lib/ethdev/ethdev_vdev.h | 2 ++
 1 file changed, 2 insertions(+)

lib/ethdev/ethdev_vdev.h +2 -0

index 364f140f91..6d94a65d97 100644
@@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device *dev, size_t private_data_size)
 {
 	struct rte_eth_dev *eth_dev;
 	const char *name = rte_vdev_device_name(dev);
+	if (name == NULL)
+		return NULL;
 
 	eth_dev = rte_eth_dev_allocate(name);
 	if (!eth_dev)

Signature · 1 line

2.25.1

Stephen Hemminger

2023-01-20 16:47 UTC #12 lore raw

Re: [PATCH v3 10/10] bus/vdev: check result of rte_vdev_device_name

lib/ethdev/ethdev_vdev.h rte_eth_vdev_allocate(struct rte_vdev_device *dev, size_t private_data_size) · line 38

Quoted 10 lines · On Thu, 19 Jan 2023 23:41:40 -0500 [email protected] wrote:

lib/ethdev/ethdev_vdev.h +2 -0

index 364f140f91..6d94a65d97 100644
@@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device *dev, size_t private_data_size)
 {
 	struct rte_eth_dev *eth_dev;
 	const char *name = rte_vdev_device_name(dev);
+	if (name == NULL)
+		return NULL;

Please add a blank line after declarations and before code. For some reason the DPDK version of checkpatch suppresses this warning.

Thomas Monjalon

2023-01-22 20:51 UTC #13 lore raw

Re: [PATCH v3 10/10] bus/vdev: check result of rte_vdev_device_name

20/01/2023 17:47, Stephen Hemminger:

Quoted 16 lines

lib/ethdev/ethdev_vdev.h rte_eth_vdev_allocate(struct rte_vdev_device *dev, size_t private_data_size) · line 38
Quoted 10 lines · On Thu, 19 Jan 2023 23:41:40 -0500 [email protected] wrote:
lib/ethdev/ethdev_vdev.h +2 -0
index 364f140f91..6d94a65d97 100644
@@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device *dev, size_t private_data_size)
 {
 	struct rte_eth_dev *eth_dev;
 	const char *name = rte_vdev_device_name(dev);
+	if (name == NULL)
+		return NULL;
Please add a blank line after declarations and before code. For some reason the DPDK version of checkpatch suppresses this warning.

The check of "name" is related to the line above,
so I don't think we should insert a blank line.

Sinan Kaya

author

2023-02-06 16:01 UTC #14 lore raw

Re: [PATCH v3 10/10] bus/vdev: check result of rte_vdev_device_name

Quoted 20 lines · Thomas Monjalon

20/01/2023 17:47, Stephen Hemminger:
Quoted 18 lines
lib/ethdev/ethdev_vdev.h rte_eth_vdev_allocate(struct rte_vdev_device · line 39
Quoted 11 lines · On Thu, 19 Jan 2023 23:41:40 -0500 [email protected] wrote:
lib/ethdev/ethdev_vdev.h +2 -0
index 364f140f91..6d94a65d97 100644
@@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device
 *dev, size_t private_data_size)
 {
 	struct rte_eth_dev *eth_dev;
 	const char *name = rte_vdev_device_name(dev);
+	if (name == NULL)
+		return NULL;
Please add a blank line after declarations and before code. For some reason the DPDK version of checkpatch suppresses this warning.

Quoted 4 lines

The check of "name" is related to the line above,
so I don't think we should insert a blank line.

While I'm waiting for maintainer feedback, let me know which way I should go. I can update this for next rev.

Thomas Monjalon

2023-02-06 18:46 UTC #15 lore raw

Re: [PATCH v3 10/10] bus/vdev: check result of rte_vdev_device_name

06/02/2023 17:01, Sinan Kaya:

Quoted 21 lines · Thomas Monjalon

Quoted 20 lines
20/01/2023 17:47, Stephen Hemminger:
Quoted 18 lines
lib/ethdev/ethdev_vdev.h rte_eth_vdev_allocate(struct rte_vdev_device · line 39
Quoted 11 lines · On Thu, 19 Jan 2023 23:41:40 -0500 [email protected] wrote:
lib/ethdev/ethdev_vdev.h +2 -0
index 364f140f91..6d94a65d97 100644
@@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device
 *dev, size_t private_data_size)
 {
 	struct rte_eth_dev *eth_dev;
 	const char *name = rte_vdev_device_name(dev);
+	if (name == NULL)
+		return NULL;
Please add a blank line after declarations and before code. For some reason the DPDK version of checkpatch suppresses this warning.

Quoted 5 lines

Quoted 2 lines
The check of "name" is related to the line above,
so I don't think we should insert a blank line.
While I'm waiting for maintainer feedback, let me know which way I should go. I can update this for next rev.

It's just a matter of taste, not a big deal.
I think you can keep it as-is.

Sinan Kaya

author

2023-02-06 16:00 UTC #16 lore raw

In reply to [email protected]

Quoted 31 lines · On Thu, 2023-01-19 at 23:41 -0500, [email protected] wrote:

From: Sinan Kaya <[email protected]>
Following up the codeql reported problems first submitted by Stephen Hemminger here:
https://lore.kernel.org/all/[email protected]/t/
Posting a series of fixes about potential null pointer accesses.
Changes from v3:
- Dropped net/tap: check if name is null
- Moved the comment to the appropriate position in ethdev: check return result of rte_eth_dev_info_get
Changes from v2:
- Remove braces around single line statements
- use NULL comparisons
Sinan Kaya (10):
ethdev: check return result of rte_eth_dev_info_get
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

Quoted 8 lines

 lib/eal/common/eal_common_memalloc.c |  5 ++++-
 lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
 lib/eal/common/malloc_elem.c         | 14 +++++++++++---
 lib/eal/common/malloc_heap.c         |  9 ++++++++-
 lib/ethdev/ethdev_vdev.h             |  2 ++
 lib/ethdev/rte_class_eth.c           |  4 +++-
 6 files changed, 37 insertions(+), 7 deletions(-)

Anatoly, any feedback on the series? You seem to be a maintainer for 2-9 patches.

Stephen Hemminger

Acked-by on the v3 series

2023-07-06 22:43 UTC #17 lore raw

In reply to [email protected]

Quoted 30 lines · On Thu, 19 Jan 2023 23:41:30 -0500 [email protected] wrote:

From: Sinan Kaya <[email protected]>
Following up the codeql reported problems first submitted by Stephen Hemminger here:
https://lore.kernel.org/all/[email protected]/t/
Posting a series of fixes about potential null pointer accesses.
Changes from v3:
- Dropped net/tap: check if name is null
- Moved the comment to the appropriate position in ethdev: check return result of rte_eth_dev_info_get
Changes from v2:
- Remove braces around single line statements
- use NULL comparisons
Sinan Kaya (10):
ethdev: check return result of rte_eth_dev_info_get
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

Quoted 7 lines

 lib/eal/common/eal_common_memalloc.c |  5 ++++-
 lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
 lib/eal/common/malloc_elem.c         | 14 +++++++++++---
 lib/eal/common/malloc_heap.c         |  9 ++++++++-
 lib/ethdev/ethdev_vdev.h             |  2 ++
 lib/ethdev/rte_class_eth.c           |  4 +++-
 6 files changed, 37 insertions(+), 7 deletions(-)

The patches are fine, only warning was from the requirement that all submitters be in .mailmap now. Do you mind if I rebase these, add a mailmap and combine the like ones together?

Acked-by Stephen Hemminger <[email protected]>

Sinan Kaya

2023-07-20 05:18 UTC #18 lore raw

Quoted 9 lines · Stephen Hemminger

Quoted 7 lines

  lib/eal/common/eal_common_memalloc.c |  5 ++++-
  lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
  lib/eal/common/malloc_elem.c         | 14 +++++++++++---
  lib/eal/common/malloc_heap.c         |  9 ++++++++-
  lib/ethdev/ethdev_vdev.h             |  2 ++
  lib/ethdev/rte_class_eth.c           |  4 +++-
  6 files changed, 37 insertions(+), 7 deletions(-)

Quoted 7 lines

The patches are fine, only warning was from the requirement that all
submitters be in .mailmap now. Do you mind if I rebase these, add a mailmap
and combine the like ones together?

Please go ahead.

Quoted 3 lines

Acked-by Stephen Hemminger <[email protected]>

Stephen Hemminger

2023-07-10 17:08 UTC #19 lore raw

v4 cover fixes for problems found by codeql analyzer

In reply to [email protected]

CodeQL is an analyzer (like Coverity) for static analysis
https://codeql.github.com/

Followup from earlier set from Sinan. Rebased to main and consolidated for easier review

Sinan Kaya (4):
ethdev: check return result of rte_eth_dev_info_get
memzone: check result of rte_fbarray_get and malloc_elem_from_data
malloc: codeql fixes
bus/vdev: check result of rte_vdev_device_name

Stephen Hemminger (1):
mailmap: add Sinan

 .mailmap                             |  1 +
 lib/eal/common/eal_common_memalloc.c |  5 ++++-
 lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
 lib/eal/common/malloc_elem.c         | 14 +++++++++++---
 lib/eal/common/malloc_heap.c         |  9 ++++++++-
 lib/ethdev/ethdev_vdev.h             |  2 ++
 lib/ethdev/rte_class_eth.c           |  4 +++-
 7 files changed, 38 insertions(+), 7 deletions(-)

Signature · 1 line

2.39.2

Stephen Hemminger

2023-07-10 17:08 UTC #20 lore raw

PATCH v4 1/5 mailmap: add Sinan

Patches get flagged as in error if mailmap missing.

Signed-off-by Stephen Hemminger <[email protected]>

---
 .mailmap | 1 +
 1 file changed, 1 insertion(+)

.mailmap +1 -0

index d200f363394d..91557940e3d9 100644
@@ -1285,6 +1285,7 @@ Simon Ellmann <[email protected]>
 Simon Horman <[email protected]> <[email protected]>
 Simon Kagstrom <[email protected]>
 Simon Kuenzer <[email protected]>
+Sinan Kaya <[email protected]>
 Siobhan Butler <[email protected]>
 Sirshak Das <[email protected]>
 Sivaprasad Tummala <[email protected]> <[email protected]>

Signature · 1 line

2.39.2

Stephen Hemminger

2023-07-10 17:08 UTC #21 lore raw

PATCH v4 2/5 ethdev: check return result of rte_eth_dev_info_get

In reply to Stephen Hemminger

From: Sinan Kaya <[email protected]>

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 <[email protected]>

Signed-off-by Stephen Hemminger <[email protected]>

Reviewed-by Morten Brørup <[email protected]>

---
 lib/ethdev/rte_class_eth.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

lib/ethdev/rte_class_eth.c +3 -1

index b61dae849dce..05c100efbba3 100644
@@ -50,8 +50,10 @@ eth_mac_cmp(const char *key __rte_unused,
 	if (rte_ether_unformat_addr(value, &mac) < 0)
 		return -1; /* invalid devargs value */
 
+	if (rte_eth_dev_info_get(data->port_id, &dev_info) < 0)
+		return -1;
+
 	/* Return 0 if devargs MAC is matching one of the device MACs. */
-	rte_eth_dev_info_get(data->port_id, &dev_info);
 	for (index = 0; index < dev_info.max_mac_addrs; index++)
 		if (rte_is_same_ether_addr(&mac, &data->mac_addrs[index]))
 			return 0;

Signature · 1 line

2.39.2

Andrew Rybchenko

Reviewed-by on v4 2/5

2023-12-16 10:06 UTC #22 lore raw

Re: [PATCH v4 2/5] ethdev: check return result of rte_eth_dev_info_get

Quoted 8 lines · Stephen Hemminger

From: Sinan Kaya <[email protected]>
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 <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>
Reviewed-by Morten Brørup <[email protected]>

Reviewed-by Andrew Rybchenko <[email protected]>

Andrew Rybchenko

2023-12-16 10:08 UTC #23 lore raw

Re: [PATCH v4 2/5] ethdev: check return result of rte_eth_dev_info_get

Quoted 11 lines · Andrew Rybchenko

Quoted 8 lines · Stephen Hemminger
From: Sinan Kaya <[email protected]>
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 <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>
Reviewed-by Morten Brørup <[email protected]>
Reviewed-by Andrew Rybchenko <[email protected]>

except of patch summary which should not mention function name

Stephen Hemminger

2023-07-10 17:08 UTC #24 lore raw

PATCH v4 3/5 memzone: check result of rte_fbarray_get and malloc_elem_from_data

In reply to Stephen Hemminger

From: Sinan Kaya <[email protected]>

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

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 <[email protected]>

Signed-off-by Stephen Hemminger <[email protected]>

---
 lib/eal/common/eal_common_memzone.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

lib/eal/common/eal_common_memzone.c +9 -1

index 1f3e7014995b..c96053b41e63 100644
@@ -77,7 +77,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 != NULL) && (mz->addr != NULL) &&
 				!strncmp(name, mz->name, RTE_MEMZONE_NAMESIZE))
 			return mz;
 		i = rte_fbarray_find_next_used(arr, i + 1);

@@ -206,6 +206,10 @@ memzone_reserve_aligned_thread_unsafe(const char *name, size_t len,
 	}
 
 	struct malloc_elem *elem = malloc_elem_from_data(mz_addr);
+	if (elem == NULL) {
+		rte_errno = ENOSPC;
+		return NULL;
+	}
 
 	/* fill the zone in config */
 	mz_idx = rte_fbarray_find_next_free(arr, 0);

@@ -395,6 +399,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 == NULL) {
+		RTE_LOG(DEBUG, EAL, "Skipping bad memzone\n");
+		return;
+	}
 
 	do {
 		fprintf(f, "  addr: %p iova: 0x%" PRIx64 " "

Signature · 1 line

2.39.2

Stephen Hemminger

2023-07-10 17:08 UTC #25 lore raw

PATCH v4 4/5 malloc: codeql fixes

In reply to Stephen Hemminger

From: Sinan Kaya <[email protected]>

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

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

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

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

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

In malloc_elem_alloc result of call to elem_start_pt is dereferenced here and may be null.

Signed-off-by Sinan Kaya <[email protected]>

Signed-off-by Stephen Hemminger <[email protected]>

---
 lib/eal/common/eal_common_memalloc.c |  5 ++++-
 lib/eal/common/malloc_elem.c         | 14 +++++++++++---
 lib/eal/common/malloc_heap.c         |  9 ++++++++-
 3 files changed, 23 insertions(+), 5 deletions(-)

lib/eal/common/eal_common_memalloc.c +4 -1

index ab04479c1cc5..24506f8447d7 100644
@@ -126,6 +126,9 @@ 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 == NULL)
+			return false;
+
 		cur = ms->iova;
 		expected = cur + pgsz;

@@ -137,7 +140,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 != NULL) && (ms->iova != expected))
 				return false;
 		}
 	}

lib/eal/common/malloc_elem.c +11 -3

index 619c040aa3e8..443ae26d283a 100644
@@ -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 == NULL)
+		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 != NULL) {
+				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

@@ -430,6 +435,9 @@ malloc_elem_alloc(struct malloc_elem *elem, size_t size, unsigned align,
 {
 	struct malloc_elem *new_elem = elem_start_pt(elem, size, align, bound,
 			contig);
+	if (new_elem == NULL)
+		return NULL;
+
 	const size_t old_elem_size = (uintptr_t)new_elem - (uintptr_t)elem;
 	const size_t trailer_size = elem->size - old_elem_size - size -
 		MALLOC_ELEM_OVERHEAD;

lib/eal/common/malloc_heap.c +8 -1

index 6b6cf9174cd3..0abaaa8c57f8 100644
@@ -97,6 +97,8 @@ 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 == NULL)
+		return NULL;
 
 	malloc_elem_free_list_insert(elem);

@@ -321,6 +323,8 @@ 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 == NULL)
+		return NULL;
 
 	/* check if we wanted contiguous memory but didn't get it */
 	if (contig && !eal_memalloc_is_contig(msl, map_addr, alloc_sz)) {

@@ -897,6 +901,9 @@ malloc_heap_free(struct malloc_elem *elem)
 	/* anything after this is a bonus */
 	ret = 0;
 
+	if (elem == NULL)
+		goto free_unlock;
+
 	/* ...of which we can't avail if we are in legacy mode, or if this is an
 	 * externally allocated segment.
 	 */

@@ -935,7 +942,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 != NULL) && (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);
 		}

Signature · 1 line

2.39.2

Stephen Hemminger

2023-07-10 17:08 UTC #26 lore raw

PATCH v4 5/5 bus/vdev: check result of rte_vdev_device_name

In reply to Stephen Hemminger

From: Sinan Kaya <[email protected]>

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

Signed-off-by Sinan Kaya <[email protected]>

Signed-off-by Stephen Hemminger <[email protected]>

---
 lib/ethdev/ethdev_vdev.h | 2 ++
 1 file changed, 2 insertions(+)

lib/ethdev/ethdev_vdev.h +2 -0

index 364f140f91b5..6d94a65d975d 100644
@@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device *dev, size_t private_data_size)
 {
 	struct rte_eth_dev *eth_dev;
 	const char *name = rte_vdev_device_name(dev);
+	if (name == NULL)
+		return NULL;
 
 	eth_dev = rte_eth_dev_allocate(name);
 	if (!eth_dev)

Signature · 1 line

2.39.2

Andrew Rybchenko

Reviewed-by on v4 5/5

2023-12-16 10:10 UTC #27 lore raw

Re: [PATCH v4 5/5] bus/vdev: check result of rte_vdev_device_name

Quoted 7 lines · Stephen Hemminger

From: Sinan Kaya <[email protected]>
In rte_eth_vdev_allocate result of call to rte_vdev_device_name is dereferenced here and may be null.
Signed-off-by Sinan Kaya <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>

Patch summary should be human-readable and do not mention internals (function name).

Other than that:

Reviewed-by Andrew Rybchenko <[email protected]>

fengchengwen

2023-07-11 01:37 UTC #28 lore raw

Re: [PATCH v4 0/5] fixes for problems found by codeql analyzer

In reply to Stephen Hemminger

Quoted 11 lines · Stephen Hemminger

CodeQL is an analyzer (like Coverity) for static analysis
https://codeql.github.com/
Followup from earlier set from Sinan. Rebased to main and consolidated for easier review
Sinan Kaya (4):
ethdev: check return result of rte_eth_dev_info_get
memzone: check result of rte_fbarray_get and malloc_elem_from_data
malloc: codeql fixes
bus/vdev: check result of rte_vdev_device_name

Please add Cc.

Series-acked-by Chengwen Feng <[email protected]>

Quoted 13 lines

Stephen Hemminger (1):
mailmap: add Sinan

 .mailmap                             |  1 +
 lib/eal/common/eal_common_memalloc.c |  5 ++++-
 lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
 lib/eal/common/malloc_elem.c         | 14 +++++++++++---
 lib/eal/common/malloc_heap.c         |  9 ++++++++-
 lib/ethdev/ethdev_vdev.h             |  2 ++
 lib/ethdev/rte_class_eth.c           |  4 +++-
 7 files changed, 38 insertions(+), 7 deletions(-)