codeql fixes for various subsystems
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.
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 | 4 ++++ lib/eal/common/eal_common_memalloc.c | 4 +++- lib/eal/common/eal_common_memzone.c | 10 +++++++++- lib/eal/common/malloc_elem.c | 14 +++++++++++--- lib/eal/common/malloc_heap.c | 11 ++++++++++- lib/ethdev/ethdev_vdev.h | 2 ++ lib/ethdev/rte_class_eth.c | 4 +++- 7 files changed, 42 insertions(+), 7 deletions(-)
Signature · 1 line
2.25.1
PATCH 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 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 | 4 ++++ 1 file changed, 4 insertions(+)
drivers/net/tap/rte_eth_tap.c +4 -0
@@ -2340,6 +2340,10 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)struct rte_eth_dev *eth_dev; name =rte_vdev_device_name (dev); +if (!name) { +return -1 ; + } + params =rte_vdev_device_args (dev);memset (remote_iface,0 ,RTE_ETH_NAME_MAX_LEN );
Signature · 1 line
2.25.1
Re: [PATCH 02/11] net/tap: check if name is null
21/11/2022 21:40, [email protected]:
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev) · line 2343
Quoted 7 lines
drivers/net/tap/rte_eth_tap.c +1
-0@@ -2340,6 +2340,10 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)struct rte_eth_dev *eth_dev; name =rte_vdev_device_name (dev); +if (!name) {
Please it is preferred to check against NULL,
because name is not a boolean, thanks.
I know it's longer but it is more explicit.
Thanks for the fixes in this series.
Re: [PATCH 02/11] net/tap: check if name is null
Quoted 9 lines · Thomas Monjalon
21/11/2022 21:40, [email protected]:
Quoted 5 lines
--- a/drivers/net/tap/rte_eth_tap.c+++ b/drivers/net/tap/rte_eth_tap.c@@ -2340,6 +2340,10 @@
rte_pmd_tun_probe(struct rte_vdev_device *dev)structrte_eth_dev*eth_dev; name = rte_vdev_device_name(dev);+ if
(!name) {Please it is preferred to check against NULL,because name is not a boolean, thanks.I know it's longer but it is more explicit.
Sure, I can do that. Getting used to dpdk coding style. I wasn't sure what to do with braces on single line too. At least, I got a warning on that too.
Quoted 2 lines
Thanks for the fixes in this series.
Cheers
Re: [PATCH 02/11] net/tap: check if name is null
Quoted 17 lines · Sinan Kaya
Quoted 9 lines · Thomas Monjalon
21/11/2022 21:40,
[email protected]
<mailto:[email protected]>
:Quoted 5 lines
drivers/net/tap/rte_eth_tap.c +0
-0@@ -2340,6 +2340,10 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)struct rte_eth_dev *eth_dev;Quoted 6 lines
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev) · line 2343
Quoted 2 lines
drivers/net/tap/rte_eth_tap.c +1
-0name =rte_vdev_device_name (dev); +if (!name) {Please it is preferred to check against NULL,
because name is not a boolean, thanks.
I know it's longer but it is more explicit.
Quoted 4 lines
Sure, I can do that. Getting used to dpdk coding style. I wasn't sure what to do with braces on single line too. At least, I got a warning on that too.
DPDK coding convention is documented if it helps:
https://doc.dpdk.org/guides/contributing/coding_style.html
Quoted 7 lines
Quoted 4 lines
Thanks for the fixes in this series.
Cheers
PATCH 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 && 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) { +RTE_LOG (DEBUG ,EAL ,"Skipping bad memzone\n" ); +return ; + } do { fprintf(f, " addr: %p iova: 0x%" PRIx64 " "
Signature · 1 line
2.25.1
PATCH 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) { + 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
PATCH 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 | 3 +++ 1 file changed, 3 insertions(+)
lib/eal/common/malloc_heap.c +3 -0
@@ -97,6 +97,9 @@ 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) { +return NULL ; + }malloc_elem_free_list_insert (elem);
Signature · 1 line
2.25.1
PATCH 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 | 3 +++ 1 file changed, 3 insertions(+)
lib/eal/common/malloc_heap.c +3 -0
@@ -324,6 +324,9 @@ 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) { +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
PATCH 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 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
lib/eal/common/eal_common_memalloc.c +3 -1
@@ -126,6 +126,8 @@ 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) +return false; cur = ms->iova; expected = cur + pgsz;
@@ -137,7 +139,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 && (ms->iova != expected))return false; } }
Signature · 1 line
2.25.1
PATCH 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) +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) { + 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
@@ -932,7 +932,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 && (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
PATCH 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
@@ -894,6 +894,9 @@ malloc_heap_free(struct malloc_elem *elem)/* anything after this is a bonus */ ret =0 ; +if (!elem) + 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