[dpdk-dev] [PATCH] eal: fix core number validation

[dpdk-dev] [PATCH] eal: fix core number validation

[Hari Kumar Vemula]

When incorrect core value or range provided,
as part of -l command line option, a crash occurs.

Added valid range checks to fix the crash.

Fixes: d888cb8b9613 ("eal: add core list input format")
Cc: stable@dpdk.org

Signed-off-by: Hari Kumar Vemula <hari.kumarx.vemula@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index e31eca5c0..ea6bb508b 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -602,6 +602,14 @@ eal_parse_corelist(const char *corelist)
 			max = idx;
 			if (min == RTE_MAX_LCORE)
 				min = idx;
+			if ((unsigned int)idx >= cfg->lcore_count ||
+					(unsigned int)min >= cfg->lcore_count) {
+				RTE_LOG(ERR, EAL,
+					"Invalid core number given core range should be(0-%u)\n",
+					cfg->lcore_count);
+				return -1;
+			}
+
 			for (idx = min; idx <= max; idx++) {
 				if (cfg->lcore_role[idx] != ROLE_RTE) {
 					cfg->lcore_role[idx] = ROLE_RTE;
-- 
2.17.2

Re: [dpdk-dev] [PATCH] eal: fix core number validation

[David Marchand]

On Thu, Dec 20, 2018 at 11:01 AM Hari Kumar Vemula <
hari.kumarx.vemula@intel.com> wrote:

> When incorrect core value or range provided,
> as part of -l command line option, a crash occurs.
>
> Added valid range checks to fix the crash.
>
> Fixes: d888cb8b9613 ("eal: add core list input format")
> Cc: stable@dpdk.org
>
> Signed-off-by: Hari Kumar Vemula <hari.kumarx.vemula@intel.com>
>

Thanks for reporting.
I agree that some validation steps are missing, but I tried a little bit
and did not reproduce a crash.

On my 8 cores system:
[dmarchan@dmarchan dpdk]$ ./master/app/testpmd --no-huge -l
567890,567891,567892 -m 512 --log-level *:debug -- -i --total-num-mbufs 2048
[...]
EAL: Support maximum 128 logical core(s) by configuration.
EAL: Detected 8 lcore(s)
[...]
EAL: invalid core list

Usage: ./master/app/testpmd [options]
etc...


[dmarchan@dmarchan dpdk]$ ./master/app/testpmd --no-huge -l 2,3,-1 -m 512
--log-level *:debug -- -i --total-num-mbufs 2048
[...]
EAL: Support maximum 128 logical core(s) by configuration.
EAL: Detected 8 lcore(s)
EAL: Detected 1 NUMA nodes
[...]
Done
testpmd>
Bye...

Idem with

[dmarchan@dmarchan dpdk]$ ./master/app/testpmd --no-huge -l 2,3,567890 -m
512 --log-level *:debug -- -i --total-num-mbufs 2048
[...]
EAL: Support maximum 128 logical core(s) by configuration.
EAL: Detected 8 lcore(s)
EAL: Detected 1 NUMA nodes
[...]
Done
testpmd>
Bye...


Since you have identified a potential crash, can you give an example of
such a crash ?
Besides, we have tests that check arguments, so an update of the test would
be nice.

Thanks.


-- 
David Marchand

Re: [dpdk-dev] [PATCH] eal: fix core number validation

[Burakov, Anatoly]

On 21-Dec-18 8:27 AM, David Marchand wrote:
> On Thu, Dec 20, 2018 at 11:01 AM Hari Kumar Vemula <
> hari.kumarx.vemula@intel.com> wrote:
> 
>> When incorrect core value or range provided,
>> as part of -l command line option, a crash occurs.
>>
>> Added valid range checks to fix the crash.
>>
>> Fixes: d888cb8b9613 ("eal: add core list input format")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Hari Kumar Vemula <hari.kumarx.vemula@intel.com>
>>
> 
> Thanks for reporting.
> I agree that some validation steps are missing, but I tried a little bit
> and did not reproduce a crash.
> 
> On my 8 cores system:
> [dmarchan@dmarchan dpdk]$ ./master/app/testpmd --no-huge -l
> 567890,567891,567892 -m 512 --log-level *:debug -- -i --total-num-mbufs 2048
> [...]
> EAL: Support maximum 128 logical core(s) by configuration.
> EAL: Detected 8 lcore(s)
> [...]
> EAL: invalid core list
> 
> Usage: ./master/app/testpmd [options]
> etc...
> 
> 
> [dmarchan@dmarchan dpdk]$ ./master/app/testpmd --no-huge -l 2,3,-1 -m 512
> --log-level *:debug -- -i --total-num-mbufs 2048
> [...]
> EAL: Support maximum 128 logical core(s) by configuration.
> EAL: Detected 8 lcore(s)
> EAL: Detected 1 NUMA nodes
> [...]
> Done
> testpmd>
> Bye...
> 
> Idem with
> 
> [dmarchan@dmarchan dpdk]$ ./master/app/testpmd --no-huge -l 2,3,567890 -m
> 512 --log-level *:debug -- -i --total-num-mbufs 2048
> [...]
> EAL: Support maximum 128 logical core(s) by configuration.
> EAL: Detected 8 lcore(s)
> EAL: Detected 1 NUMA nodes
> [...]
> Done
> testpmd>
> Bye...
> 
> 
> Since you have identified a potential crash, can you give an example of
> such a crash ?
> Besides, we have tests that check arguments, so an update of the test would
> be nice.
> 
> Thanks.
> 

I believe these lcore numbers are used to index the lcore list later, 
which would cause out-of-bounds access, which may or may not cause a 
crash, depending on how lucky you get.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] eal: fix core number validation

[David Marchand]

On Fri, Dec 21, 2018 at 10:19 AM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:

> On 21-Dec-18 8:27 AM, David Marchand wrote:
> > Since you have identified a potential crash, can you give an example of
> > such a crash ?
> > Besides, we have tests that check arguments, so an update of the test
> would
> > be nice.
>
> I believe these lcore numbers are used to index the lcore list later,
> which would cause out-of-bounds access, which may or may not cause a
> crash, depending on how lucky you get.
>

Sure, that and the fact that my testpmd was doing nothing with them :-).
Anyway, the unit tests missed this case, so we need an update.

And the more I look at those string parsing, the more I think that the
service cores parsing has the same issue (copy/paste yay ;-)).


-- 
David Marchand

Re: [dpdk-dev] [PATCH] eal: fix core number validation

[Vemula, Hari KumarX]

Hi,

From: David Marchand [mailto:david.marchand@redhat.com]
Sent: Friday, December 21, 2018 2:58 PM
To: Burakov, Anatoly <anatoly.burakov@intel.com>; Vemula, Hari KumarX <hari.kumarx.vemula@intel.com>
Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] eal: fix core number validation


On Fri, Dec 21, 2018 at 10:19 AM Burakov, Anatoly <anatoly.burakov@intel.com<mailto:anatoly.burakov@intel.com>> wrote:
On 21-Dec-18 8:27 AM, David Marchand wrote:
> Since you have identified a potential crash, can you give an example of
> such a crash ?
> Besides, we have tests that check arguments, so an update of the test would
> be nice.

I believe these lcore numbers are used to index the lcore list later,
which would cause out-of-bounds access, which may or may not cause a
crash, depending on how lucky you get.

Sure, that and the fact that my testpmd was doing nothing with them :-).
Anyway, the unit tests missed this case, so we need an update.
And the more I look at those string parsing, the more I think that the service cores parsing has the same issue (copy/paste yay ;-)).

DPDK EAL doesn’t have eal parameter valid/invalid core number check.
For example, if we use following command to launch dpdk testpmd sample, it will return segmentation fault directly:
./x86_64-native-linuxapp-gcc/app/testpmd -l 9999999 -n 4 –-i
EAL: Detected 48 lcore(s)
EAL: Detected 2 NUMA nodes
Segmentation fault (core dumped)

  UT is present for -l command line option to validate core number, but there is no check for higher core list number like above.

--
Hari.


--
David Marchand
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

Re: [dpdk-dev] [PATCH] eal: fix core number validation

[David Marchand]

On Thu, Dec 20, 2018 at 11:01 AM Hari Kumar Vemula <
hari.kumarx.vemula@intel.com> wrote:

> When incorrect core value or range provided,
> as part of -l command line option, a crash occurs.
>
> Added valid range checks to fix the crash.
>
> Fixes: d888cb8b9613 ("eal: add core list input format")
> Cc: stable@dpdk.org
>
> Signed-off-by: Hari Kumar Vemula <hari.kumarx.vemula@intel.com>
> ---
>  lib/librte_eal/common/eal_common_options.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/lib/librte_eal/common/eal_common_options.c
> b/lib/librte_eal/common/eal_common_options.c
> index e31eca5c0..ea6bb508b 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -602,6 +602,14 @@ eal_parse_corelist(const char *corelist)
>                         max = idx;
>                         if (min == RTE_MAX_LCORE)
>                                 min = idx;
> +                       if ((unsigned int)idx >= cfg->lcore_count ||
> +                                       (unsigned int)min >=
> cfg->lcore_count) {
>

Rather than check those conditions here, check directly after parsing the
input string.

Those casts will have side effects with negative values and the use of
current strtoul is suspicious to me.
You should switch to strtol, check for negative values and for the max
value according to cfg->lcore_count.



> +                               RTE_LOG(ERR, EAL,
> +                                       "Invalid core number given core
> range should be(0-%u)\n",
> +                                       cfg->lcore_count);
>

This log message will be a duplicate with the message in
eal_parse_common_option() so please drop this.

We could add the range in the current log message in
eal_parse_common_option(), but please, the range is [0, cfg->lcore_count -
1].


+                               return -1;
> +                       }
> +
>                         for (idx = min; idx <= max; idx++) {
>                                 if (cfg->lcore_role[idx] != ROLE_RTE) {
>                                         cfg->lcore_role[idx] = ROLE_RTE;
>


-- 
David Marchand