codeql fixes for various subsystems
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
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
@@ -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
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
@@ -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
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
@@ -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
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
@@ -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
Re: [PATCH v2 04/11] memzone: check result of malloc_elem_from_data
2022-11-21 17:32 (UTC-0500), [email protected]:
Quoted 6 lines
From: Sinan Kaya <[email protected]>
In
memzone_reserve_aligned_thread_unsaferesult of call tomalloc_elem_from_datais dereferenced here and may be null.Signed-off-by Sinan Kaya <[email protected]>
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
@@ -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
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_memoryresult of call tomalloc_elem_join_adjacent_freeis 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).
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
@@ -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
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_heapresult of call torte_mem_virt2memseg_listis 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 +0
-0
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.
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
@@ -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
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_contigresult of call torte_fbarray_getis 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 +3
-0@@ -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.
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
@@ -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
@@ -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
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_contigresult of call torte_mem_virt2memsegis 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@@ -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
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.
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
@@ -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
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_freeresult of call tomalloc_elem_freeis 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?
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
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