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

  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>
---
v5:
- update message
v4:
- remove check condition from loop
v3:
- add check condition inside and outside the loop
v2:
- add message information
---
 lib/librte_eal/linuxapp/eal/eal_memory.c | 8 ++++++++
 1 file changed, 8 insertions(+)
  

2016-06-16 16:52, 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

Please use this formatting:
Coverity issue: 13282

> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
> ---
> v5:
> - update message
> v4:
> - remove check condition from loop
> v3:
> - add check condition inside and outside the loop
> v2:
> - add message information

The changelog is OK.
Please use --in-reply-to when making a new revision to keep them
in the same thread.

> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> @@ -1301,6 +1301,14 @@ rte_eal_hugepage_init(void)
>  			break;
>  		}
>  

No newline needed here. The check is directly related to the
previous loop.

> +	if (j >= RTE_MAX_MEMSEG) {

It is out of the scope of this patch but I REALLY HATE this variable j.
Considering a more meaningful rename would be a nice patch.

> +		RTE_LOG(ERR, EAL,
> +			"All memory segments exhausted by IVSHMEM. "

There is no evidence that it is related to IVSHMEM.
"Not enough memory segments." would be more appropriate.

> +			"Try recompiling with larger RTE_MAX_MEMSEG "
> +			"then current %d\n", RTE_MAX_MEMSEG);

then -> than
  

On 20/06/2016 10:14, Thomas Monjalon wrote:
>> +		RTE_LOG(ERR, EAL,
>> +			"All memory segments exhausted by IVSHMEM. "
> There is no evidence that it is related to IVSHMEM.
> "Not enough memory segments." would be more appropriate.

Actually we would hit this issue when all memsegs have been used by IVSHMEM.
So I think the message is accurate.

Am I missing something?

Sergio
  

2016-06-20 10:38, Sergio Gonzalez Monroy:
> On 20/06/2016 10:14, Thomas Monjalon wrote:
> >> +		RTE_LOG(ERR, EAL,
> >> +			"All memory segments exhausted by IVSHMEM. "
> > There is no evidence that it is related to IVSHMEM.
> > "Not enough memory segments." would be more appropriate.
> 
> Actually we would hit this issue when all memsegs have been used by IVSHMEM.
> So I think the message is accurate.

I think it's saner to avoid mixing "potential root cause of a use case" and
"accurate description of the error".
One day, the root cause could be different and the message will become wrong.
Here there is not enough memory segment.
  

On 20/06/2016 11:09, Thomas Monjalon wrote:
> 2016-06-20 10:38, Sergio Gonzalez Monroy:
>> On 20/06/2016 10:14, Thomas Monjalon wrote:
>>>> +		RTE_LOG(ERR, EAL,
>>>> +			"All memory segments exhausted by IVSHMEM. "
>>> There is no evidence that it is related to IVSHMEM.
>>> "Not enough memory segments." would be more appropriate.
>> Actually we would hit this issue when all memsegs have been used by IVSHMEM.
>> So I think the message is accurate.
> I think it's saner to avoid mixing "potential root cause of a use case" and
> "accurate description of the error".
> One day, the root cause could be different and the message will become wrong.
> Here there is not enough memory segment.
>

Right.
So the whole point of doing the check before the loop was to display
the error message with its specific cause.

I would think that if the code changes and the message is not accurate then
it should also be updated.

So if folks prefer a more generic error message probably we don't need 
the check
before the loop and just change the check condition inside the loop that 
would
end up printing the generic error message (after the loop).

Basically v1 would do that.
http://dpdk.org/dev/patchwork/patch/12241/

Sergio
  

Hi Thomas,

As I understand Sergio suggested to come back to the solution similar to v1.
Could you comment or better take decision which solution should be applied, please.

Best Regards,
Sławomir 


>-----Original Message-----
>From: Gonzalez Monroy, Sergio
>Sent: Monday, June 20, 2016 1:29 PM
>To: Thomas Monjalon <thomas.monjalon@6wind.com>
>Cc: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>;
>dev@dpdk.org; david.marchand@6wind.com
>Subject: Re: [dpdk-dev] [PATCH v5] eal: out-of-bounds write
>
>On 20/06/2016 11:09, Thomas Monjalon wrote:
>> 2016-06-20 10:38, Sergio Gonzalez Monroy:
>>> On 20/06/2016 10:14, Thomas Monjalon wrote:
>>>>> +		RTE_LOG(ERR, EAL,
>>>>> +			"All memory segments exhausted by IVSHMEM. "
>>>> There is no evidence that it is related to IVSHMEM.
>>>> "Not enough memory segments." would be more appropriate.
>>> Actually we would hit this issue when all memsegs have been used by
>IVSHMEM.
>>> So I think the message is accurate.
>> I think it's saner to avoid mixing "potential root cause of a use
>> case" and "accurate description of the error".
>> One day, the root cause could be different and the message will become
>wrong.
>> Here there is not enough memory segment.
>>
>
>Right.
>So the whole point of doing the check before the loop was to display the error
>message with its specific cause.
>
>I would think that if the code changes and the message is not accurate then it
>should also be updated.
>
>So if folks prefer a more generic error message probably we don't need the
>check before the loop and just change the check condition inside the loop that
>would end up printing the generic error message (after the loop).
>
>Basically v1 would do that.
>http://dpdk.org/dev/patchwork/patch/12241/
>
>Sergio
  

2016-07-21 12:01, Mrozowicz, SlawomirX:
> Hi Thomas,
> 
> As I understand Sergio suggested to come back to the solution similar to v1.
> Could you comment or better take decision which solution should be applied, please.
> 
> Best Regards,
> Sławomir 
> 
> 
> >-----Original Message-----
> >From: Gonzalez Monroy, Sergio
> >On 20/06/2016 11:09, Thomas Monjalon wrote:
> >> 2016-06-20 10:38, Sergio Gonzalez Monroy:
> >>> On 20/06/2016 10:14, Thomas Monjalon wrote:
> >>>>> +		RTE_LOG(ERR, EAL,
> >>>>> +			"All memory segments exhausted by IVSHMEM. "
> >>>> There is no evidence that it is related to IVSHMEM.
> >>>> "Not enough memory segments." would be more appropriate.
> >>> Actually we would hit this issue when all memsegs have been used by
> >IVSHMEM.
> >>> So I think the message is accurate.
> >> I think it's saner to avoid mixing "potential root cause of a use
> >> case" and "accurate description of the error".
> >> One day, the root cause could be different and the message will become
> >wrong.
> >> Here there is not enough memory segment.
> >>
> >
> >Right.
> >So the whole point of doing the check before the loop was to display the error
> >message with its specific cause.
> >
> >I would think that if the code changes and the message is not accurate then it
> >should also be updated.
> >
> >So if folks prefer a more generic error message probably we don't need the
> >check before the loop and just change the check condition inside the loop that
> >would end up printing the generic error message (after the loop).
> >
> >Basically v1 would do that.
> >http://dpdk.org/dev/patchwork/patch/12241/

At this point of 16.07 we can apply the v1 if you agree.
The message about IVSHMEM will be totally wrong when the ivshmem specific
code will be removed.
If we need more error messages, feel free to send another patch.