[PATCH v2 00/11] codeql fixes for various subsystems

[PATCH v2 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.

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(-)

-- 
2.25.1

[PATCH v2 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 v2 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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f2a6c33a19..b99439e4f2 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -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);
 
-- 
2.25.1

[PATCH v2 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..8d472505eb 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 != 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 " "
-- 
2.25.1

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

[PATCH v2 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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index d7c410b786..503e551bf9 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -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);
 
-- 
2.25.1

[PATCH v2 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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index 503e551bf9..3f41430e42 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -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)) {
-- 
2.25.1

[PATCH v2 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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/eal/common/eal_common_memalloc.c b/lib/eal/common/eal_common_memalloc.c
index ab04479c1c..24506f8447 100644
--- a/lib/eal/common/eal_common_memalloc.c
+++ b/lib/eal/common/eal_common_memalloc.c
@@ -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;
 		}
 	}
-- 
2.25.1

[PATCH v2 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..8f49812846 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 == 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
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index 3f41430e42..88270ce4d2 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -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);
 		}
-- 
2.25.1

[PATCH v2 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 88270ce4d2..6eb6fcda5e 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -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.
 	 */
-- 
2.25.1

Re: [PATCH v2 00/11] codeql fixes for various subsystems

[David Marchand]

Hello,

On Mon, Nov 21, 2022 at 11:32 PM <okaya@kernel.org> wrote:
>
> 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.
>
> 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.
-- 
David Marchand

Re: [PATCH v2 00/11] codeql fixes for various subsystems

[Sinan Kaya]

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

On Tue, 2022-11-22 at 16:24 +0100, David Marchand wrote:
> Thanks for the fixes, but it looks like the v2 series is truncated.
> 
> Can you resend it so it passes through the CI?

Sure

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

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

[Dmitry Kozlyuk]

2022-11-21 17:32 (UTC-0500), okaya@kernel.org:
> 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>

Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

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

[Dmitry Kozlyuk]

2022-11-21 17:32 (UTC-0500), okaya@kernel.org:
> 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.

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).

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

[Dmitry Kozlyuk]

2022-11-21 17:32 (UTC-0500), okaya@kernel.org:
> 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 | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
> index 503e551bf9..3f41430e42 100644
> --- a/lib/eal/common/malloc_heap.c
> +++ b/lib/eal/common/malloc_heap.c
> @@ -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.

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

[Dmitry Kozlyuk]

2022-11-21 17:32 (UTC-0500), okaya@kernel.org:
> 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 | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/eal/common/eal_common_memalloc.c b/lib/eal/common/eal_common_memalloc.c
> index ab04479c1c..24506f8447 100644
> --- a/lib/eal/common/eal_common_memalloc.c
> +++ b/lib/eal/common/eal_common_memalloc.c
> @@ -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;
>  		}
>  	}

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

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

[Dmitry Kozlyuk]

2022-11-21 17:32 (UTC-0500), okaya@kernel.org:
> 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..8f49812846 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 == 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
> diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
> index 3f41430e42..88270ce4d2 100644
> --- a/lib/eal/common/malloc_heap.c
> +++ b/lib/eal/common/malloc_heap.c
> @@ -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.

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

[Dmitry Kozlyuk]

2022-11-21 17:32 (UTC-0500), okaya@kernel.org:
> From: Sinan Kaya <okaya@kernel.org>
> 
> 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?