[2/2] app/testpmd: fix stack overflow for EEPROM display

Message ID 20220120025931.574106-3-stevex.yang@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series add module EEPROM ops for ice |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot: build success github build: passed
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Steve Yang Jan. 20, 2022, 2:59 a.m. UTC
  When the size of EEPROM exceeds the default thread stack size(8MB),
e.g.: 10Mb size, it will be cashed with stack overflow.

Allocate the data of EPPROM information on the heap.

Fixes: 6b67721dee2a ("app/testpmd: add EEPROM command")

Signed-off-by: Steve Yang <stevex.yang@intel.com>
---
 app/test-pmd/config.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)
  

Comments

Singh, Aman Deep Feb. 2, 2022, 10 a.m. UTC | #1
Hi Steve
The patch looks ok to me.

On 1/20/2022 8:29 AM, Steve Yang wrote:
> When the size of EEPROM exceeds the default thread stack size(8MB),
> e.g.: 10Mb size, it will be cashed with stack overflow.
Just spelling: 10Mb/10MB, cashed/crashed
Can even rephrase, like: it will crash due to stack overflow
> Allocate the data of EPPROM information on the heap.
>
> Fixes: 6b67721dee2a ("app/testpmd: add EEPROM command")
>
> Signed-off-by: Steve Yang<stevex.yang@intel.com>

Acked-by: Aman Singh <aman.deep.singh@intel.com>

> ---
>   app/test-pmd/config.c | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 1722d6c8f8..e812f57151 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -912,10 +912,15 @@ port_eeprom_display(portid_t port_id)
>   		return;
>   	}
>   
> -	char buf[len_eeprom];
>   	einfo.offset = 0;
>   	einfo.length = len_eeprom;
> -	einfo.data = buf;
> +	einfo.data = calloc(1, len_eeprom);
> +	if (!einfo.data) {
> +		fprintf(stderr,
> +			"Allocation of port %u eeprom data failed\n",
> +			port_id);
> +		return;
> +	}
>   
>   	ret = rte_eth_dev_get_eeprom(port_id, &einfo);
>   	if (ret != 0) {
> @@ -933,10 +938,12 @@ port_eeprom_display(portid_t port_id)
>   			fprintf(stderr, "Unable to get EEPROM: %d\n", ret);
>   			break;
>   		}
> +		free(einfo.data);
>   		return;
>   	}
>   	rte_hexdump(stdout, "hexdump", einfo.data, einfo.length);
>   	printf("Finish -- Port: %d EEPROM length: %d bytes\n", port_id, len_eeprom); + free(einfo.data); } void @@ -972,10 +979,15 
> @@ port_module_eeprom_display(portid_t port_id) return; } - char 
> buf[minfo.eeprom_len]; einfo.offset = 0; einfo.length = 
> minfo.eeprom_len; - einfo.data = buf; + einfo.data = calloc(1, 
> minfo.eeprom_len); + if (!einfo.data) { + fprintf(stderr, + "Allocation of port %u eeprom data failed\n",
> +			port_id);
> +		return;
> +	}
>   
>   	ret = rte_eth_dev_get_module_eeprom(port_id, &einfo);
>   	if (ret != 0) {
> @@ -994,11 +1006,13 @@ port_module_eeprom_display(portid_t port_id)
>   				ret);
>   			break;
>   		}
> +		free(einfo.data);
>   		return;
>   	}
>   
>   	rte_hexdump(stdout, "hexdump", einfo.data, einfo.length);
>   	printf("Finish -- Port: %d MODULE EEPROM length: %d bytes\n", port_id, einfo.length);
> +	free(einfo.data);
>   }
>   
>   int
  
Ferruh Yigit Feb. 3, 2022, 1:15 p.m. UTC | #2
On 2/2/2022 10:00 AM, Singh, Aman Deep wrote:
> Hi Steve
> The patch looks ok to me.
> 
> On 1/20/2022 8:29 AM, Steve Yang wrote:
>> When the size of EEPROM exceeds the default thread stack size(8MB),
>> e.g.: 10Mb size, it will be cashed with stack overflow.
> Just spelling: 10Mb/10MB, cashed/crashed
> Can even rephrase, like: it will crash due to stack overflow

fixed while merging

>> Allocate the data of EPPROM information on the heap.
>>
>> Fixes: 6b67721dee2a ("app/testpmd: add EEPROM command")
>>
>> Signed-off-by: Steve Yang<stevex.yang@intel.com>
> 
> Acked-by: Aman Singh <aman.deep.singh@intel.com>
> 

Tested-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/main, thanks.
  

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 1722d6c8f8..e812f57151 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -912,10 +912,15 @@  port_eeprom_display(portid_t port_id)
 		return;
 	}
 
-	char buf[len_eeprom];
 	einfo.offset = 0;
 	einfo.length = len_eeprom;
-	einfo.data = buf;
+	einfo.data = calloc(1, len_eeprom);
+	if (!einfo.data) {
+		fprintf(stderr,
+			"Allocation of port %u eeprom data failed\n",
+			port_id);
+		return;
+	}
 
 	ret = rte_eth_dev_get_eeprom(port_id, &einfo);
 	if (ret != 0) {
@@ -933,10 +938,12 @@  port_eeprom_display(portid_t port_id)
 			fprintf(stderr, "Unable to get EEPROM: %d\n", ret);
 			break;
 		}
+		free(einfo.data);
 		return;
 	}
 	rte_hexdump(stdout, "hexdump", einfo.data, einfo.length);
 	printf("Finish -- Port: %d EEPROM length: %d bytes\n", port_id, len_eeprom);
+	free(einfo.data);
 }
 
 void
@@ -972,10 +979,15 @@  port_module_eeprom_display(portid_t port_id)
 		return;
 	}
 
-	char buf[minfo.eeprom_len];
 	einfo.offset = 0;
 	einfo.length = minfo.eeprom_len;
-	einfo.data = buf;
+	einfo.data = calloc(1, minfo.eeprom_len);
+	if (!einfo.data) {
+		fprintf(stderr,
+			"Allocation of port %u eeprom data failed\n",
+			port_id);
+		return;
+	}
 
 	ret = rte_eth_dev_get_module_eeprom(port_id, &einfo);
 	if (ret != 0) {
@@ -994,11 +1006,13 @@  port_module_eeprom_display(portid_t port_id)
 				ret);
 			break;
 		}
+		free(einfo.data);
 		return;
 	}
 
 	rte_hexdump(stdout, "hexdump", einfo.data, einfo.length);
 	printf("Finish -- Port: %d MODULE EEPROM length: %d bytes\n", port_id, einfo.length);
+	free(einfo.data);
 }
 
 int