Skip to contentSkip to conversation

codeql fixes for various subsystems

PATCH RESEND Superseded [email protected] 4 participants 21 messages Nov 22, 2022 – Jan 19, 2023 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 RESEND 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
Acked-by on v2 1/11
#2 lore raw

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

22/11/2022 16:30, [email protected]:

Quoted 2 lines

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 eth_mac_cmp(const char *key __rte_unused, · line 59

Quoted 8 lines
lib/ethdev/rte_class_eth.c +3 -1
 	/* 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;

You would get more attention if you Cc the maintainers. You can use this git option: --cc-cmd devtools/get-maintainer.sh

author
#3 lore raw

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

Quoted 18 lines · Thomas Monjalon

22/11/2022 16:30,
[email protected]
:

Quoted 3 lines

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.

[...]

Quoted 8 lines
       /* Return 0 if devargs MAC is matching one of the device

MACs. */

lib/ethdev/rte_class_eth.c +3 -1
-     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-
Quoted 10 lines
Quoted 2 lines
Quoted 1 line

mac_addrs[index]))

                       return 0;

Acked-by: Thomas Monjalon <
[email protected]

Quoted 1 line

Thanks.

Quoted 4 lines

You would get more attention if you Cc the maintainers.

You can use this git option: --cc-cmd devtools/get-maintainer.sh

Good to know that we have get-maintainer script.

#4 lore raw

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

In reply to [email protected]
Quoted 15 lines · On 11/22/2022 3:30 PM, [email protected] wrote:

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 +0 -0
index 838b3a8f9f..8165e5adc0 100644

lib/ethdev/rte_class_eth.c eth_mac_cmp(const char *key __rte_unused, · line 59

Quoted 10 lines
lib/ethdev/rte_class_eth.c +3 -1
@@ -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;

Minor issue, but before patch, comment about MAC address match applies to block of code that gets dev_info and does MAC compare. After patch, get dev_info is separate code block and comment doesn't really apply to that block, although it is possible to understand what comment is referring, still I think better to move it down to the block that does MAC compare.

#5 lore raw

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

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

Quoted 15 lines · On 11/22/2022 3:30 PM, [email protected] wrote:

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 +0 -0
index f2a6c33a19..b99439e4f2 100644

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev) · line 2348

Quoted 9 lines
drivers/net/tap/rte_eth_tap.c +3 -0
@@ -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);
 

Yes, technically 'rte_vdev_device_name()' can return NULL, but call stack is like this:

`
vdev_probe_all_drivers() //in 'vdev.c'
name = rte_vdev_device_name(dev);
if (vdev_parse(name, &driver))

    return -1;

ret = driver->probe(dev); //it is 'rte_pmd_tun_probe()'
`

I assume this is highlighted by a tool but in practice 'rte_vdev_device_name()' should not return NULL, and there are many other location this check is missing.

Although it doesn't hurt, I think we can skip the check since it is unnecessary.

author
#7 lore raw

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

Quoted 5 lines · Ferruh Yigit

I assume this is highlighted by a tool but in practice

'rte_vdev_device_name()' should not return NULL, and there are many

other location this check is missing.

That's correct. Warning was found by codeql static analysis tool. When I search the codebase,
I do see a mixture of null checks and no checks for this function. We can ignore this patch, if you
feel strongly against it.

#8 lore raw

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

PATCH RESEND 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
#10 lore raw

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

PATCH RESEND 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
#12 lore raw

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

PATCH RESEND 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
#14 lore raw

PATCH RESEND 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

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

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

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

---
 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
author
#17 lore raw
In reply to [email protected]
Quoted 17 lines · On Tue, 2022-11-22 at 10:30 -0500, [email protected] wrote:

From: Sinan Kaya <[email protected]>
Following up the codeql reported problems first submittedby 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

Quoted 9 lines
 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(-)

Checking to see if I need to cover any feedback.

author
#19 lore raw
Quoted 9 lines · Dmitry Kozlyuk

2022-12-13 11:43 (UTC-0500), Sinan Kaya:

Quoted 1 line

Checking to see if I need to cover any feedback.

There was some feedback to pre-resend v2 that is not visible on patchwork:

http://inbox.dpdk.org/dev/CAJFAV8yEe9awya3OVLuqavfW0MzonSQJQ-K+2q4WampmyrjEGg@mail.gmail.com/

Yes, I sent it again with RESEND tag this time on this series. Just checking to see if it went through this time.
I do see the RESEND series on patchwork.

author
#20 lore raw
Quoted 7 lines · Sinan Kaya
Quoted 6 lines · Dmitry Kozlyuk

2022-12-13 11:43 (UTC-0500), Sinan Kaya:

Quoted 1 line

Checking to see if I need to cover any feedback.

There was some feedback to pre-resend v2 that is not visible on patchwork:

Quoted 10 lines
Quoted 4 lines

http://inbox.dpdk.org/dev/CAJFAV8yEe9awya3OVLuqavfW0MzonSQJQ-K+2q4WampmyrjEGg@mail.gmail.com/

Yes, I sent it again with RESEND tag this time on this series. Just checking to see if it went through this time.
I do see the RESEND series on patchwork.

I was planning to send the fixes in batches hoping this one to merge first. I guess I'll post the remaning bugfixes using a different series.