[PATCH v5] eal: out-of-bounds write

[PATCH v5] 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>
---
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(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 5b9132c..ffe069c 100644
--- 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;
 		}
 
+	if (j >= RTE_MAX_MEMSEG) {
+		RTE_LOG(ERR, EAL,
+			"All memory segments exhausted by IVSHMEM. "
+			"Try recompiling with larger RTE_MAX_MEMSEG "
+			"then current %d\n", RTE_MAX_MEMSEG);
+		return -ENOMEM;
+	}
+
 	for (i = 0; i < nr_hugefiles; i++) {
 		new_memseg = 0;
 
-- 
1.9.1

Re: [PATCH v5] eal: out-of-bounds write

[Thomas Monjalon]

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

Re: [PATCH v5] eal: out-of-bounds write

[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.

Am I missing something?

Sergio

Re: [PATCH v5] eal: out-of-bounds write

[Thomas Monjalon]

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.

Re: [PATCH v5] eal: out-of-bounds write

[Sergio Gonzalez Monroy]

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

Re: [PATCH v5] eal: out-of-bounds write

[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
>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

Re: [PATCH v5] eal: out-of-bounds write

[Thomas Monjalon]

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.