[dpdk-dev] [PATCH v3] eal: out-of-bounds write

[dpdk-dev] [PATCH v3] eal: out-of-bounds write

[Slawomir Mrozowicz]

Overrunning array mcfg->memseg of 256 44-byte elements
at element index 257 using index j.
Fixed by add condition with message information.

Fixes: af75078fece3 ("first public release")
Coverity ID 13282

Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 5b9132c..6a2daf5 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1301,6 +1301,15 @@ rte_eal_hugepage_init(void)
 			break;
 		}
 
+	if (j >= RTE_MAX_MEMSEG) {
+		RTE_LOG(ERR, EAL,
+			"Failed: all memsegs used by ivshmem.\n"
+			"Current %d is not enough.\n"
+			"Please either increase the RTE_MAX_MEMSEG\n",
+			RTE_MAX_MEMSEG);
+		return -ENOMEM;
+	}
+
 	for (i = 0; i < nr_hugefiles; i++) {
 		new_memseg = 0;
 
@@ -1333,8 +1342,14 @@ rte_eal_hugepage_init(void)
 
 		if (new_memseg) {
 			j += 1;
-			if (j == RTE_MAX_MEMSEG)
-				break;
+			if (j >= RTE_MAX_MEMSEG) {
+				RTE_LOG(ERR, EAL,
+					"Failed: all memsegs used by ivshmem.\n"
+					"Current %d is not enough.\n"
+					"Please either increase the RTE_MAX_MEMSEG\n",
+					RTE_MAX_MEMSEG);
+				return -ENOMEM;
+			}
 
 			mcfg->memseg[j].phys_addr = hugepage[i].physaddr;
 			mcfg->memseg[j].addr = hugepage[i].final_va;
-- 
1.9.1

Re: [dpdk-dev] [PATCH v3] eal: out-of-bounds write

[Sergio Gonzalez Monroy]

On 14/06/2016 10:02, Slawomir Mrozowicz wrote:
> Overrunning array mcfg->memseg of 256 44-byte elements
> at element index 257 using index j.
> Fixed by add condition with message information.
>
> Fixes: af75078fece3 ("first public release")
> Coverity ID 13282
>
> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
> ---
>   lib/librte_eal/linuxapp/eal/eal_memory.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
> index 5b9132c..6a2daf5 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> @@ -1301,6 +1301,15 @@ rte_eal_hugepage_init(void)
>   			break;
>   		}
>   
> +	if (j >= RTE_MAX_MEMSEG) {
> +		RTE_LOG(ERR, EAL,
> +			"Failed: all memsegs used by ivshmem.\n"
> +			"Current %d is not enough.\n"
> +			"Please either increase the RTE_MAX_MEMSEG\n",
> +			RTE_MAX_MEMSEG);
> +		return -ENOMEM;
> +	}
> +
>   	for (i = 0; i < nr_hugefiles; i++) {
>   		new_memseg = 0;
>   
> @@ -1333,8 +1342,14 @@ rte_eal_hugepage_init(void)
>   
>   		if (new_memseg) {
>   			j += 1;
> -			if (j == RTE_MAX_MEMSEG)
> -				break;
> +			if (j >= RTE_MAX_MEMSEG) {
> +				RTE_LOG(ERR, EAL,
> +					"Failed: all memsegs used by ivshmem.\n"
> +					"Current %d is not enough.\n"
> +					"Please either increase the RTE_MAX_MEMSEG\n",
> +					RTE_MAX_MEMSEG);
> +				return -ENOMEM;
> +			}
>   

I don't think you need to change anything inside the for loop.
As it is in the patch, the error message is not accurate.
What I tried to say in my previous review was to not touch the loop and 
just do the check before the loop.
There is an error message after the loop which is correct for all cases 
except the case this patch is
addressing by checking before the loop.

Sergio

>   			mcfg->memseg[j].phys_addr = hugepage[i].physaddr;
>   			mcfg->memseg[j].addr = hugepage[i].final_va;