Skip to contentSkip to conversation

codeql fixes for various subsystems

PATCH Superseded [email protected] 3 participants 18 messages Nov 21, 2022 – Nov 22, 2022 Network drivers

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

v2 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 v1:
- Remove braces around single line statements
- use NULL comparisons

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        |  3 +++
 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, 40 insertions(+), 7 deletions(-)
Signature · 1 line
2.25.1
#1 lore raw

PATCH v2 1/11 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.

---
 lib/ethdev/rte_class_eth.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
lib/ethdev/rte_class_eth.c +3 -1
index 838b3a8f9f..8165e5adc0 100644
@@ -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;
Signature · 1 line
2.25.1
#2 lore raw

PATCH v2 2/11 net/tap: check if name is null

In reply to [email protected]

From: Sinan Kaya <[email protected]>

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

---
 drivers/net/tap/rte_eth_tap.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/net/tap/rte_eth_tap.c +3 -0
index f2a6c33a19..b99439e4f2 100644
@@ -2340,6 +2340,9 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	struct rte_eth_dev *eth_dev;
 
 	name = rte_vdev_device_name(dev);
+	if (name == NULL)
+		return -1;
+
 	params = rte_vdev_device_args(dev);
 	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
 
Signature · 1 line
2.25.1
#3 lore raw

PATCH v2 3/11 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.

---
 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
#4 lore raw

PATCH v2 4/11 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.

---
 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
#6 lore raw

PATCH v2 5/11 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.

---
 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
#7 lore raw

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

2022-11-21 17:32 (UTC-0500), [email protected]:

Quoted 4 lines

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.

It may not:
"malloc_elem_join_adjacent_free()" never returns NULL by definition. Would annotating "malloc_elem_join_adjacent_free()" result (and maybe the argument too)
convince codeql that the check is not needed?

A comment to the series:

I'm against adding extra checks *only* to silence some tool,
not because they're overly defensive,
but because they misrepresent the code assumptions,
making the understanding harder.
Returning false if assumptions are broken is arguably no better then crashing, because this means that either the internal state is inconsistent or the caller has supplied invalid arguments (logical error up the stack).

#8 lore raw

PATCH v2 6/11 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.

---
 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
#9 lore raw

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

2022-11-21 17:32 (UTC-0500), [email protected]:

Quoted 15 lines

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.

---
 lib/eal/common/malloc_heap.c | 2 ++
 1 file changed, 2 insertions(+)
lib/eal/common/malloc_heap.c +0 -0
index 503e551bf9..3f41430e42 100644

lib/eal/common/malloc_heap.c alloc_pages_on_heap(struct malloc_heap *heap, uint64_t pg_sz, size_t elt_size, · line 327

Quoted 5 lines
lib/eal/common/malloc_heap.c +2 -0
@@ -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;

It is not really possible, because the memory lock is held, so "map_addr" cannot be unmapped/remapped concurrently, and "ms" belongs to some MSL by definition of memseg. RTE_ASSERT() can be added for clarity.

#10 lore raw

PATCH v2 7/11 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.

---
 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
#11 lore raw

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

2022-11-21 17:32 (UTC-0500), [email protected]:

Quoted 25 lines

From: Sinan Kaya <[email protected]>

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

---
 lib/eal/common/eal_common_memalloc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
lib/eal/common/eal_common_memalloc.c +3 -0
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;
 

lib/eal/common/eal_common_memalloc.c eal_memalloc_is_contig(const struct rte_memseg_list *msl, void *start, · line 146

Quoted 8 lines
lib/eal/common/eal_common_memalloc.c +1 -1
@@ -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;
 		}
 	}

Invariant: "msl->memseg_arr" elements for existing memsegs are used. RTE_ASSERT(rte_fbarray_is_used(&msl->memseg_arr, ...)) would be sufficient.

#12 lore raw

PATCH v2 8/11 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.

---
 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
#13 lore raw

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

2022-11-21 17:32 (UTC-0500), [email protected]:

Quoted 45 lines

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.

---
 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 +0 -0
index 3f41430e42..88270ce4d2 100644

lib/eal/common/malloc_heap.c malloc_heap_free(struct malloc_elem *elem) · line 936

Quoted 8 lines
lib/eal/common/malloc_heap.c +1 -1
@@ -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);
 		}

In these three places "ms" or "tmp" are from the MSL by construction. I think RTE_ASSERT() would be sufficient.

#14 lore raw

PATCH v2 9/11 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.

---
 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
#15 lore raw

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

2022-11-21 17:32 (UTC-0500), [email protected]:

Quoted 4 lines

From: Sinan Kaya <[email protected]>

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

It may not: "malloc_elem_free()" never returns NULL by definition:
it takes a valid busy element and returns a valid free element. How about annotating the function instead?

#16 lore raw
In reply to [email protected]

Hello,

Quoted 6 lines · On Mon, Nov 21, 2022 at 11:32 PM <[email protected]> wrote:

From: Sinan Kaya <[email protected]>

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

Quoted 7 lines

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

Posting a series of fixes about potential null pointer accesses.

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

Thanks for the fixes, but it looks like the v2 series is truncated. Can you resend it so it passes through the CI?

Thanks.

Signature · 1 line
David Marchand
author
#17 lore raw
Quoted 3 lines · David Marchand

Thanks for the fixes, but it looks like the v2 series is truncated.

Can you resend it so it passes through the CI?

Sure