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

  Fix issue reported by Coverity.

Coverity ID 13282: Out-of-bounds write
overrun-local: Overrunning array mcfg->memseg of 256 44-byte elements
at element index 257 using index j.

Fixes: af75078fece3 ("first public release")

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

On Tue, Apr 26, 2016 at 09:44:47AM +0200, Slawomir Mrozowicz wrote:
> Fix issue reported by Coverity.
> 
> Coverity ID 13282: Out-of-bounds write
> overrun-local: Overrunning array mcfg->memseg of 256 44-byte elements
> at element index 257 using index j.
> 
> Fixes: af75078fece3 ("first public release")
> 
> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
> index 5b9132c..1e737e4 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> @@ -1333,7 +1333,7 @@ rte_eal_hugepage_init(void)
>  
>  		if (new_memseg) {
>  			j += 1;
> -			if (j == RTE_MAX_MEMSEG)
> +			if (j >= RTE_MAX_MEMSEG)
>  				break;
>  
>  			mcfg->memseg[j].phys_addr = hugepage[i].physaddr;
> -- 

This does appear to be a valid fix for the issue. However, looking at the code,
it appears that the only way we could actually hit the problem is if 
j == RTE_MAX_MEMSEG on exiting the previous loop. Would a check there be a better
fix for this issue (or perhaps we want both fixes).

Thoughts?

/Bruce
  

On 26/04/2016 09:53, Bruce Richardson wrote:
> On Tue, Apr 26, 2016 at 09:44:47AM +0200, Slawomir Mrozowicz wrote:
>> Fix issue reported by Coverity.
>>
>> Coverity ID 13282: Out-of-bounds write
>> overrun-local: Overrunning array mcfg->memseg of 256 44-byte elements
>> at element index 257 using index j.
>>
>> Fixes: af75078fece3 ("first public release")
>>
>> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
>> ---
>>   lib/librte_eal/linuxapp/eal/eal_memory.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
>> index 5b9132c..1e737e4 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
>> @@ -1333,7 +1333,7 @@ rte_eal_hugepage_init(void)
>>   
>>   		if (new_memseg) {
>>   			j += 1;
>> -			if (j == RTE_MAX_MEMSEG)
>> +			if (j >= RTE_MAX_MEMSEG)
>>   				break;
>>   
>>   			mcfg->memseg[j].phys_addr = hugepage[i].physaddr;
>> -- 
> This does appear to be a valid fix for the issue. However, looking at the code,
> it appears that the only way we could actually hit the problem is if
> j == RTE_MAX_MEMSEG on exiting the previous loop. Would a check there be a better
> fix for this issue (or perhaps we want both fixes).
>
> Thoughts?

It doesn't make sense to go into the loop if we don't have free memsegs.
Either way we should print the error indicating that we reached MAX_MEMSEG.

Sergio


> /Bruce
  

>-----Original Message-----
>From: Gonzalez Monroy, Sergio
>Sent: Tuesday, April 26, 2016 11:44 AM
>To: Richardson, Bruce <bruce.richardson@intel.com>; Mrozowicz, SlawomirX
><slawomirx.mrozowicz@intel.com>
>Cc: david.marchand@6wind.com; dev@dpdk.org
>Subject: Re: [dpdk-dev] [PATCH] eal: out-of-bounds write
>
>On 26/04/2016 09:53, Bruce Richardson wrote:
>> On Tue, Apr 26, 2016 at 09:44:47AM +0200, Slawomir Mrozowicz wrote:
>>> Fix issue reported by Coverity.
>>>
>>> Coverity ID 13282: Out-of-bounds write
>>> overrun-local: Overrunning array mcfg->memseg of 256 44-byte elements
>>> at element index 257 using index j.
>>>
>>> Fixes: af75078fece3 ("first public release")
>>>
>>> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
>>> ---
>>>   lib/librte_eal/linuxapp/eal/eal_memory.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
>>> b/lib/librte_eal/linuxapp/eal/eal_memory.c
>>> index 5b9132c..1e737e4 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
>>> @@ -1333,7 +1333,7 @@ rte_eal_hugepage_init(void)
>>>
>>>   		if (new_memseg) {
>>>   			j += 1;
>>> -			if (j == RTE_MAX_MEMSEG)
>>> +			if (j >= RTE_MAX_MEMSEG)
>>>   				break;
>>>
>>>   			mcfg->memseg[j].phys_addr = hugepage[i].physaddr;
>>> --
>> This does appear to be a valid fix for the issue. However, looking at
>> the code, it appears that the only way we could actually hit the
>> problem is if j == RTE_MAX_MEMSEG on exiting the previous loop. Would
>> a check there be a better fix for this issue (or perhaps we want both fixes).
>>
>> Thoughts?
>
>It doesn't make sense to go into the loop if we don't have free memsegs.
>Either way we should print the error indicating that we reached
>MAX_MEMSEG.
>
>Sergio
>

It is possible to add additional checking available memseg before the loop. 
In this case it will be checked twice before loop and inside loop. 
In my opinion it is not necessary.

Anyway it is valuable to add in line 1336 error message if the MAX_MEMSEG is reached.

Sławomir

>
>> /Bruce