[dpdk-dev] librte_cmdline: FreeBSD Fix oveflow when size of command result structure is greater than BUFSIZ

  When using test-pmd with flow director in FreeBSD, the application will
segfault/Bus error while parsing the command-line. This is due to how
each commands result structure is represented during parsing, where the offsets
for each tokens value is stored in a character array(char result_buf[BUFSIZ])
in cmdline_parse()(./lib/librte_cmdline/cmdline_parse.c).

The overflow occurs where BUFSIZ is less than the size of a commands result
structure, in this case "struct cmd_pkt_filter_result"
(app/test-pmd/cmdline.c) is 1088 bytes and BUFSIZ on FreeBSD is 1024 bytes as
opposed to 8192 bytes on Linux.

This patch removes the OS dependency on BUFSIZ and defines and uses a
library #define CMDLINE_PARSE_RESULT_BUFSIZE 8192

The problem can be reproduced by running test-pmd on FreeBSD:
./testpmd -c 0x3 -n 4 -- -i --portmask=0x3 --pkt-filter-mode=perfect
And adding a filter:
add_perfect_filter 0 udp src 192.168.0.0 1024 dst 192.168.0.0 1024 flexbytes
0x800 vlan 0 queue 0 soft 0x17

Signed-off-by: Alan Carew <alan.carew@intel.com>
---
 lib/librte_cmdline/cmdline_parse.c | 2 +-
 lib/librte_cmdline/cmdline_parse.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)
  

A comment on my own patch.

Making the size of result_buf consistent across each OS and keeping it as large
as the Linux BUFSIZ(8192) doesn't really address the core issue.

In the event that a user of librte_cmdline creates a custom context with a
result structure > 8192 bytes then this problem will occur again, though 
somewhat unlikely, as the minimum number of the largest type would be 64 x 
cmdline_fixed_string_t types within a result structure, at its current size.

There is no checking of overflow, I would be tempted to add a runtime check in
cmdline_parse()/match_inst(), however I would be more comfortable with a build
time check for this type of problem.

Due to the opaque handling of user defined contexts there is no obvious way to
do this at build time.

Thoughts?

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Alan Carew
> Sent: Monday, October 20, 2014 4:23 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] librte_cmdline: FreeBSD Fix oveflow when size of
> command result structure is greater than BUFSIZ
> 
> When using test-pmd with flow director in FreeBSD, the application will
> segfault/Bus error while parsing the command-line. This is due to how
> each commands result structure is represented during parsing, where the offsets
> for each tokens value is stored in a character array(char result_buf[BUFSIZ])
> in cmdline_parse()(./lib/librte_cmdline/cmdline_parse.c).
> 
> The overflow occurs where BUFSIZ is less than the size of a commands result
> structure, in this case "struct cmd_pkt_filter_result"
> (app/test-pmd/cmdline.c) is 1088 bytes and BUFSIZ on FreeBSD is 1024 bytes as
> opposed to 8192 bytes on Linux.
> 
> This patch removes the OS dependency on BUFSIZ and defines and uses a
> library #define CMDLINE_PARSE_RESULT_BUFSIZE 8192
> 
> The problem can be reproduced by running test-pmd on FreeBSD:
> ./testpmd -c 0x3 -n 4 -- -i --portmask=0x3 --pkt-filter-mode=perfect
> And adding a filter:
> add_perfect_filter 0 udp src 192.168.0.0 1024 dst 192.168.0.0 1024 flexbytes
> 0x800 vlan 0 queue 0 soft 0x17
> 
> Signed-off-by: Alan Carew <alan.carew@intel.com>
> ---
>  lib/librte_cmdline/cmdline_parse.c | 2 +-
>  lib/librte_cmdline/cmdline_parse.h | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_cmdline/cmdline_parse.c
> b/lib/librte_cmdline/cmdline_parse.c
> index 940480d..29f1afd 100644
> --- a/lib/librte_cmdline/cmdline_parse.c
> +++ b/lib/librte_cmdline/cmdline_parse.c
> @@ -219,7 +219,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
>  	unsigned int inst_num=0;
>  	cmdline_parse_inst_t *inst;
>  	const char *curbuf;
> -	char result_buf[BUFSIZ];
> +	char result_buf[CMDLINE_PARSE_RESULT_BUFSIZE];
>  	void (*f)(void *, struct cmdline *, void *) = NULL;
>  	void *data = NULL;
>  	int comment = 0;
> diff --git a/lib/librte_cmdline/cmdline_parse.h
> b/lib/librte_cmdline/cmdline_parse.h
> index f18836d..dae53ba 100644
> --- a/lib/librte_cmdline/cmdline_parse.h
> +++ b/lib/librte_cmdline/cmdline_parse.h
> @@ -80,6 +80,9 @@ extern "C" {
>  #define CMDLINE_PARSE_COMPLETE_AGAIN    1
>  #define CMDLINE_PARSE_COMPLETED_BUFFER  2
> 
> +/* maximum buffer size for parsed result */
> +#define CMDLINE_PARSE_RESULT_BUFSIZE 8192
> +
>  /**
>   * Stores a pointer to the ops struct, and the offset: the place to
>   * write the parsed result in the destination structure.
> --
> 1.9.3
  

Hi Alan,

2014-10-20 15:26, Carew, Alan:
> A comment on my own patch.
> 
> Making the size of result_buf consistent across each OS and keeping it as large
> as the Linux BUFSIZ(8192) doesn't really address the core issue.
> 
> In the event that a user of librte_cmdline creates a custom context with a
> result structure > 8192 bytes then this problem will occur again, though 
> somewhat unlikely, as the minimum number of the largest type would be 64 x 
> cmdline_fixed_string_t types within a result structure, at its current size.
> 
> There is no checking of overflow, I would be tempted to add a runtime check in
> cmdline_parse()/match_inst(), however I would be more comfortable with a build
> time check for this type of problem.
> 
> Due to the opaque handling of user defined contexts there is no obvious way to
> do this at build time.
> 
> Thoughts?

librte_cmdline derivates from libcmdline written by Olivier Matz: 
	http://git.droids-corp.org/?p=libcmdline.git
Maybe there are some fixes to take here, and probably Olivier will have
some good insights.


> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Alan Carew
> > Sent: Monday, October 20, 2014 4:23 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH] librte_cmdline: FreeBSD Fix oveflow when size of
> > command result structure is greater than BUFSIZ
> > 
> > When using test-pmd with flow director in FreeBSD, the application will
> > segfault/Bus error while parsing the command-line. This is due to how
> > each commands result structure is represented during parsing, where the offsets
> > for each tokens value is stored in a character array(char result_buf[BUFSIZ])
> > in cmdline_parse()(./lib/librte_cmdline/cmdline_parse.c).
> > 
> > The overflow occurs where BUFSIZ is less than the size of a commands result
> > structure, in this case "struct cmd_pkt_filter_result"
> > (app/test-pmd/cmdline.c) is 1088 bytes and BUFSIZ on FreeBSD is 1024 bytes as
> > opposed to 8192 bytes on Linux.
> > 
> > This patch removes the OS dependency on BUFSIZ and defines and uses a
> > library #define CMDLINE_PARSE_RESULT_BUFSIZE 8192
> > 
> > The problem can be reproduced by running test-pmd on FreeBSD:
> > ./testpmd -c 0x3 -n 4 -- -i --portmask=0x3 --pkt-filter-mode=perfect
> > And adding a filter:
> > add_perfect_filter 0 udp src 192.168.0.0 1024 dst 192.168.0.0 1024 flexbytes
> > 0x800 vlan 0 queue 0 soft 0x17
> > 
> > Signed-off-by: Alan Carew <alan.carew@intel.com>
> > ---
> >  lib/librte_cmdline/cmdline_parse.c | 2 +-
> >  lib/librte_cmdline/cmdline_parse.h | 3 +++
> >  2 files changed, 4 insertions(+), 1 deletion(-)
[...]
> > --- a/lib/librte_cmdline/cmdline_parse.c
> > +++ b/lib/librte_cmdline/cmdline_parse.c
> > @@ -219,7 +219,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
> >  	unsigned int inst_num=0;
> >  	cmdline_parse_inst_t *inst;
> >  	const char *curbuf;
> > -	char result_buf[BUFSIZ];
> > +	char result_buf[CMDLINE_PARSE_RESULT_BUFSIZE];
[...]
> > --- a/lib/librte_cmdline/cmdline_parse.h
> > +++ b/lib/librte_cmdline/cmdline_parse.h
> > @@ -80,6 +80,9 @@ extern "C" {
> >  #define CMDLINE_PARSE_COMPLETE_AGAIN    1
> >  #define CMDLINE_PARSE_COMPLETED_BUFFER  2
> > 
> > +/* maximum buffer size for parsed result */
> > +#define CMDLINE_PARSE_RESULT_BUFSIZE 8192
> > +
  

Hello Alan,

On 10/20/2014 05:26 PM, Carew, Alan wrote:
> A comment on my own patch.
> 
> Making the size of result_buf consistent across each OS and keeping it as large
> as the Linux BUFSIZ(8192) doesn't really address the core issue.
> 
> In the event that a user of librte_cmdline creates a custom context with a
> result structure > 8192 bytes then this problem will occur again, though 
> somewhat unlikely, as the minimum number of the largest type would be 64 x 
> cmdline_fixed_string_t types within a result structure, at its current size.
> 
> There is no checking of overflow, I would be tempted to add a runtime check in
> cmdline_parse()/match_inst(), however I would be more comfortable with a build
> time check for this type of problem.
> 
> Due to the opaque handling of user defined contexts there is no obvious way to
> do this at build time.
> 
> Thoughts?

Indeed, your patch does not address the core issue of the problem,
altough it's already an improvement to the current situation.

Your issue was already fixed in the latest libcmdline library by
this patch (which also includes the replacement of BUFSIZ):
http://git.droids-corp.org/?p=libcmdline.git;a=commitdiff;h=b1d5b169352e57df3fc14c51ffad4b83f3e5613f

I'm pretty sure it won't apply smoothly on the dpdk command line
library but it can probably be adapted. Ideally, the latest libcmdline
library should be [cleaned first and] merged in dpdk.org.

Regards,
Olivier
  

Hi,

On 10/27/2014 10:14 AM, Olivier MATZ wrote:
> Hello Alan,
>
> On 10/20/2014 05:26 PM, Carew, Alan wrote:
>> A comment on my own patch.
>>
>> Making the size of result_buf consistent across each OS and keeping it as large
>> as the Linux BUFSIZ(8192) doesn't really address the core issue.
>>
>> In the event that a user of librte_cmdline creates a custom context with a
>> result structure > 8192 bytes then this problem will occur again, though
>> somewhat unlikely, as the minimum number of the largest type would be 64 x
>> cmdline_fixed_string_t types within a result structure, at its current size.
>>
>> There is no checking of overflow, I would be tempted to add a runtime check in
>> cmdline_parse()/match_inst(), however I would be more comfortable with a build
>> time check for this type of problem.
>>
>> Due to the opaque handling of user defined contexts there is no obvious way to
>> do this at build time.
>>
>> Thoughts?
>
> Indeed, your patch does not address the core issue of the problem,
> altough it's already an improvement to the current situation.
>
> Your issue was already fixed in the latest libcmdline library by
> this patch (which also includes the replacement of BUFSIZ):
> http://git.droids-corp.org/?p=libcmdline.git;a=commitdiff;h=b1d5b169352e57df3fc14c51ffad4b83f3e5613f
>
> I'm pretty sure it won't apply smoothly on the dpdk command line
> library but it can probably be adapted. Ideally, the latest libcmdline
> library should be [cleaned first and] merged in dpdk.org.

Sorry, I had no time to deeply check this. I think your patch can
go in 1.8 as it's still an enhancement compared to the current
situation. We may go back on this later.

Acked-by: Olivier Matz <olivier.matz@6wind.com>
  

2014-12-03 17:05, Olivier MATZ:
> Hi,
> 
> On 10/27/2014 10:14 AM, Olivier MATZ wrote:
> > Hello Alan,
> >
> > On 10/20/2014 05:26 PM, Carew, Alan wrote:
> >> A comment on my own patch.
> >>
> >> Making the size of result_buf consistent across each OS and keeping it as large
> >> as the Linux BUFSIZ(8192) doesn't really address the core issue.
> >>
> >> In the event that a user of librte_cmdline creates a custom context with a
> >> result structure > 8192 bytes then this problem will occur again, though
> >> somewhat unlikely, as the minimum number of the largest type would be 64 x
> >> cmdline_fixed_string_t types within a result structure, at its current size.
> >>
> >> There is no checking of overflow, I would be tempted to add a runtime check in
> >> cmdline_parse()/match_inst(), however I would be more comfortable with a build
> >> time check for this type of problem.
> >>
> >> Due to the opaque handling of user defined contexts there is no obvious way to
> >> do this at build time.
> >>
> >> Thoughts?
> >
> > Indeed, your patch does not address the core issue of the problem,
> > altough it's already an improvement to the current situation.
> >
> > Your issue was already fixed in the latest libcmdline library by
> > this patch (which also includes the replacement of BUFSIZ):
> > http://git.droids-corp.org/?p=libcmdline.git;a=commitdiff;h=b1d5b169352e57df3fc14c51ffad4b83f3e5613f
> >
> > I'm pretty sure it won't apply smoothly on the dpdk command line
> > library but it can probably be adapted. Ideally, the latest libcmdline
> > library should be [cleaned first and] merged in dpdk.org.
> 
> Sorry, I had no time to deeply check this. I think your patch can
> go in 1.8 as it's still an enhancement compared to the current
> situation. We may go back on this later.
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied

Thanks
  

Hi folks and thanks,

> > Sorry, I had no time to deeply check this. I think your patch can go
> > in 1.8 as it's still an enhancement compared to the current situation.
> > We may go back on this later.
> >
> > Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Applied
> 
> Thanks
> --
> Thomas

I did post a V2 http://dpdk.org/ml/archives/dev/2014-November/007920.html.
I must have used an incorrect message ID incorrect for "--in-reply-to" though.

Regards,
Alan.
  

Hi Alan,

On 12/04/2014 03:01 PM, Carew, Alan wrote:
>>> Sorry, I had no time to deeply check this. I think your patch can go
>>> in 1.8 as it's still an enhancement compared to the current situation.
>>> We may go back on this later.
>>>
>>> Acked-by: Olivier Matz <olivier.matz@6wind.com>
>>
>> Applied
>>
>> Thanks
>> --
>> Thomas
>
> I did post a V2 http://dpdk.org/ml/archives/dev/2014-November/007920.html.
> I must have used an incorrect message ID incorrect for "--in-reply-to" though.

Sorry, I completely missed it. I'll review it ASAP.

Thanks,
Olivier
  

2014-12-04 14:01, Carew, Alan:
> I did post a V2 http://dpdk.org/ml/archives/dev/2014-November/007920.html.
> I must have used an incorrect message ID incorrect for "--in-reply-to" though.

Sorry I failed to check similar patches.
I reverted the v1 patch and wait the review to apply the v2.