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 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
@@ -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
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 torte_eth_dev_info_getis not checked, potentially leavingdev_infouninitialized.
[...]
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
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 torte_eth_dev_info_get
is not checked, potentially leavingdev_infouninitialized.[...]
Quoted 8 lines
/* Return 0 if devargs MAC is matching one of the deviceMACs. */
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.
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 torte_eth_dev_info_getis not checked, potentially leavingdev_infouninitialized.Signed-off-by Sinan Kaya <[email protected]>--- lib/ethdev/rte_class_eth.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)lib/ethdev/rte_class_eth.c +0
-0
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.
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
@@ -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
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_proberesult of call torte_vdev_device_nameis dereferenced here and may be null.Signed-off-by Sinan Kaya <[email protected]>--- drivers/net/tap/rte_eth_tap.c | 3 +++ 1 file changed, 3 insertions(+)drivers/net/tap/rte_eth_tap.c +0
-0
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.
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 manyother 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.
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
@@ -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 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
@@ -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
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
@@ -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
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
@@ -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
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
@@ -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
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
@@ -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
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
@@ -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
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
@@ -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
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
@@ -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
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 ofrte_eth_dev_info_getnet/tap: check if name is null memzone: check result ofrte_fbarray_getmemzone: check result ofmalloc_elem_from_datamalloc:malloc_elem_join_adjacent_freecan return null malloc: check result ofrte_mem_virt2memseg_listmalloc: check result ofrte_fbarray_getmalloc: check result ofrte_mem_virt2memsegmalloc:
check result ofmalloc_elem_freemalloc: check result ofelem_start_ptbus/vdev: check result ofrte_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, 40insertions(+), 7 deletions(-)
Checking to see if I need to cover any feedback.
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/
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.
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.