[PATCH] app/testpmd: fix indirect action list parameters parsing

[PATCH] app/testpmd: fix indirect action list parameters parsing

[Gregory Etelson]

Indirect actions list arguments parser was configured to place target
number into 64bit value, while the code provided 32bits memory.

The patch updated variable size for translation results.

Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 0d521159e9..cf1ca33208 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
 	struct rte_flow_action *action = ctx->object;
 	struct rte_flow_action_indirect_list *action_conf;
 	const struct indlst_conf *indlst_conf;
-	uint32_t id;
+	uint64_t id;
 	int ret;
 
 	if (!action)
@@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
 	action_conf->handle = (typeof(action_conf->handle))
 				port_action_handle_get_by_id(ctx->port, id);
 		if (!action_conf->handle) {
-			printf("no indirect list handle for id %u\n", id);
+			printf("no indirect list handle for id %"PRIu64"\n",
+			       id);
 			return -1;
 		}
 		break;
-- 
2.39.2

Re: [PATCH] app/testpmd: fix indirect action list parameters parsing

[Ferruh Yigit]

On 11/8/2023 4:34 PM, Gregory Etelson wrote:
> Indirect actions list arguments parser was configured to place target
> number into 64bit value, while the code provided 32bits memory.
> 

Hi Gregory,

Can you please give more details why 'id' needs to be 64 bits, with
callstack or usecase etc?
And please describe what is the observed problem with current code?


Inside 'parse_indlst_id2ptr()',
'parse_int()' can work or 32bits and 64bits variables, so that one is OK.
But both 'port_action_handle_get_by_id()' &
'indirect_action_list_conf_get()' gets 'id' as parameter and they get
32bits argument, when 'id' is 64bit won't it will be cast to 32bits and
loose data, should those functions needs to be updated as well.



> The patch updated variable size for translation results.
> 
> Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> ---
>  app/test-pmd/cmdline_flow.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 0d521159e9..cf1ca33208 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
>  	struct rte_flow_action *action = ctx->object;
>  	struct rte_flow_action_indirect_list *action_conf;
>  	const struct indlst_conf *indlst_conf;
> -	uint32_t id;
> +	uint64_t id;
>  	int ret;
>  
>  	if (!action)
> @@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
>  	action_conf->handle = (typeof(action_conf->handle))
>  				port_action_handle_get_by_id(ctx->port, id);
>  		if (!action_conf->handle) {
> -			printf("no indirect list handle for id %u\n", id);
> +			printf("no indirect list handle for id %"PRIu64"\n",
> +			       id);
>  			return -1;
>  		}
>  		break;

Re: [PATCH] app/testpmd: fix indirect action list parameters parsing

[Etelson, Gregory]

Hello Ferruh,

>> Indirect actions list arguments parser was configured to place target
>> number into 64bit value, while the code provided 32bits memory.
>>
>
> Hi Gregory,
>
> Can you please give more details why 'id' needs to be 64 bits, with
> callstack or usecase etc?
> And please describe what is the observed problem with current code?
>

In rte_flow.h, struct rte_flow_action_indirect_list::handle is a pointer.

Testpmd ACTION_INDIRECT_LIST_HANDLE and ACTION_INDIRECT_LIST_CONF tokens
define arguments size as uintptr_t.

On 64 bits system, defining the id variable as 32 bits value
corrupted parse_indlst_id2ptr stack.

I'll change the id definition to uintptr_t to match token in v2.

Regards,
Gregory

>
> Inside 'parse_indlst_id2ptr()',
> 'parse_int()' can work or 32bits and 64bits variables, so that one is OK.
> But both 'port_action_handle_get_by_id()' &
> 'indirect_action_list_conf_get()' gets 'id' as parameter and they get
> 32bits argument, when 'id' is 64bit won't it will be cast to 32bits and
> loose data, should those functions needs to be updated as well.
>
>
>
>> The patch updated variable size for translation results.
>>
>> Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
>> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
>> ---
>>  app/test-pmd/cmdline_flow.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>> index 0d521159e9..cf1ca33208 100644
>> --- a/app/test-pmd/cmdline_flow.c
>> +++ b/app/test-pmd/cmdline_flow.c
>> @@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
>>       struct rte_flow_action *action = ctx->object;
>>       struct rte_flow_action_indirect_list *action_conf;
>>       const struct indlst_conf *indlst_conf;
>> -     uint32_t id;
>> +     uint64_t id;
>>       int ret;
>>
>>       if (!action)
>> @@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
>>       action_conf->handle = (typeof(action_conf->handle))
>>                               port_action_handle_get_by_id(ctx->port, id);
>>               if (!action_conf->handle) {
>> -                     printf("no indirect list handle for id %u\n", id);
>> +                     printf("no indirect list handle for id %"PRIu64"\n",
>> +                            id);
>>                       return -1;
>>               }
>>               break;
>
>

Re: [PATCH] app/testpmd: fix indirect action list parameters parsing

[Ferruh Yigit]

On 11/9/2023 6:22 PM, Etelson, Gregory wrote:
> Hello Ferruh,
> 
>>> Indirect actions list arguments parser was configured to place target
>>> number into 64bit value, while the code provided 32bits memory.
>>>
>>
>> Hi Gregory,
>>
>> Can you please give more details why 'id' needs to be 64 bits, with
>> callstack or usecase etc?
>> And please describe what is the observed problem with current code?
>>
> 
> In rte_flow.h, struct rte_flow_action_indirect_list::handle is a pointer.
> 
> Testpmd ACTION_INDIRECT_LIST_HANDLE and ACTION_INDIRECT_LIST_CONF tokens
> define arguments size as uintptr_t.
> 
> On 64 bits system, defining the id variable as 32 bits value
> corrupted parse_indlst_id2ptr stack.
> 

I can't see how stack corruption can happen, can you please provide call
stack and flow command?


> I'll change the id definition to uintptr_t to match token in v2.
> 
> Regards,
> Gregory
> 
>>
>> Inside 'parse_indlst_id2ptr()',
>> 'parse_int()' can work or 32bits and 64bits variables, so that one is OK.
>> But both 'port_action_handle_get_by_id()' &
>> 'indirect_action_list_conf_get()' gets 'id' as parameter and they get
>> 32bits argument, when 'id' is 64bit won't it will be cast to 32bits and
>> loose data, should those functions needs to be updated as well.
>>

Can you please reply to above question, about changing 'id' type impact
to other functions using it?


>>
>>
>>> The patch updated variable size for translation results.
>>>
>>> Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
>>> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
>>> ---
>>>  app/test-pmd/cmdline_flow.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>>> index 0d521159e9..cf1ca33208 100644
>>> --- a/app/test-pmd/cmdline_flow.c
>>> +++ b/app/test-pmd/cmdline_flow.c
>>> @@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx,
>>> const struct token *token,
>>>       struct rte_flow_action *action = ctx->object;
>>>       struct rte_flow_action_indirect_list *action_conf;
>>>       const struct indlst_conf *indlst_conf;
>>> -     uint32_t id;
>>> +     uint64_t id;
>>>       int ret;
>>>
>>>       if (!action)
>>> @@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx,
>>> const struct token *token,
>>>       action_conf->handle = (typeof(action_conf->handle))
>>>                               port_action_handle_get_by_id(ctx->port,
>>> id);
>>>               if (!action_conf->handle) {
>>> -                     printf("no indirect list handle for id %u\n", id);
>>> +                     printf("no indirect list handle for id
>>> %"PRIu64"\n",
>>> +                            id);
>>>                       return -1;
>>>               }
>>>               break;
>>
>>

Re: [PATCH] app/testpmd: fix indirect action list parameters parsing

[Etelson, Gregory]

Hello Ferruh,

>>>> Indirect actions list arguments parser was configured to place target
>>>> number into 64bit value, while the code provided 32bits memory.
>>>>
>>>
>>> Hi Gregory,
>>>
>>> Can you please give more details why 'id' needs to be 64 bits, with
>>> callstack or usecase etc?
>>> And please describe what is the observed problem with current code?
>>>
>>
>> In rte_flow.h, struct rte_flow_action_indirect_list::handle is a pointer.
>>
>> Testpmd ACTION_INDIRECT_LIST_HANDLE and ACTION_INDIRECT_LIST_CONF tokens
>> define arguments size as uintptr_t.
>>
>> On 64 bits system, defining the id variable as 32 bits value
>> corrupted parse_indlst_id2ptr stack.
>>
>
> I can't see how stack corruption can happen, can you please provide call
> stack and flow command?
>

To reproduce the crash buildtype must be release or debugoptimized.
The crash will not reproduce with the debug builds.

Testpmd commands I use:

dpdk-testpmd -a ${PCI_ADDR},dv_flow_en=2,representor=vf0-1 -- -i

port stop all
flow configure 0 queues_number 12 queues_size 256
flow configure 1 queues_number 12 queues_size 256
flow configure 2 queues_number 12 queues_size 256
port start all
start

set raw_encap 0 eth dst is 00:16:3e:52:bd:37 src is 00:16:3e:6e:16:e0 type 
is 2048 has_vlan is 0 / ipv4 src is 110.240.52.255 dst is 189.68.183.147 
proto is 17 fragment_offset is 0 packet_id is 1 tos is 102 ttl is 189 
version_ihl is 69 / udp src is 56800 dst is 4789 / vxlan vni is 3 / 
end_set
set sample_actions 0 represented_port ethdev_port_id 0 / end

flow indirect_action 0 create action_id 5 transfer list actions sample 
ratio 1 index 0 / represented_port ethdev_port_id 2 / end
flow actions_template 0 create transfer actions_template_id 6 template 
indirect_list handle 5 / end mask  indirect_list handle 5 / end

Result:
*** stack smashing detected ***: terminated

The corruption occurred in `parse_int()` called from 
`parse_indlst_id2ptr()`.

Inside `parse_int()` the arg parameter referenced 8 bytes of memory while 
the target buffer was 4 bytes allocated on caller optimized stack:

(gdb) p *arg
$1 = { ... size = 8, ...}


>>> Inside 'parse_indlst_id2ptr()',
>>> 'parse_int()' can work or 32bits and 64bits variables, so that one is OK.
>>> But both 'port_action_handle_get_by_id()' &
>>> 'indirect_action_list_conf_get()' gets 'id' as parameter and they get
>>> 32bits argument, when 'id' is 64bit won't it will be cast to 32bits and
>>> loose data, should those functions needs to be updated as well.
>>>
>
> Can you please reply to above question, about changing 'id' type impact
> to other functions using it?
>

I've missed that.
Need to re-think.

Regards,
Gregory

>
>>>
>>>
>>>> The patch updated variable size for translation results.
>>>>
>>>> Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
>>>> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
>>>> ---
>>>>  app/test-pmd/cmdline_flow.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>>>> index 0d521159e9..cf1ca33208 100644
>>>> --- a/app/test-pmd/cmdline_flow.c
>>>> +++ b/app/test-pmd/cmdline_flow.c
>>>> @@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx,
>>>> const struct token *token,
>>>>       struct rte_flow_action *action = ctx->object;
>>>>       struct rte_flow_action_indirect_list *action_conf;
>>>>       const struct indlst_conf *indlst_conf;
>>>> -     uint32_t id;
>>>> +     uint64_t id;
>>>>       int ret;
>>>>
>>>>       if (!action)
>>>> @@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx,
>>>> const struct token *token,
>>>>       action_conf->handle = (typeof(action_conf->handle))
>>>>                               port_action_handle_get_by_id(ctx->port,
>>>> id);
>>>>               if (!action_conf->handle) {
>>>> -                     printf("no indirect list handle for id %u\n", id);
>>>> +                     printf("no indirect list handle for id
>>>> %"PRIu64"\n",
>>>> +                            id);
>>>>                       return -1;
>>>>               }
>>>>               break;
>>>
>>>
>
>

Re: [PATCH] app/testpmd: fix indirect action list parameters parsing

[Ferruh Yigit]

On 11/9/2023 7:57 PM, Etelson, Gregory wrote:
> Hello Ferruh,
> 
>>>>> Indirect actions list arguments parser was configured to place target
>>>>> number into 64bit value, while the code provided 32bits memory.
>>>>>
>>>>
>>>> Hi Gregory,
>>>>
>>>> Can you please give more details why 'id' needs to be 64 bits, with
>>>> callstack or usecase etc?
>>>> And please describe what is the observed problem with current code?
>>>>
>>>
>>> In rte_flow.h, struct rte_flow_action_indirect_list::handle is a
>>> pointer.
>>>
>>> Testpmd ACTION_INDIRECT_LIST_HANDLE and ACTION_INDIRECT_LIST_CONF tokens
>>> define arguments size as uintptr_t.
>>>
>>> On 64 bits system, defining the id variable as 32 bits value
>>> corrupted parse_indlst_id2ptr stack.
>>>
>>
>> I can't see how stack corruption can happen, can you please provide call
>> stack and flow command?
>>
> 
> To reproduce the crash buildtype must be release or debugoptimized.
> The crash will not reproduce with the debug builds.
> 
> Testpmd commands I use:
> 
> dpdk-testpmd -a ${PCI_ADDR},dv_flow_en=2,representor=vf0-1 -- -i
> 
> port stop all
> flow configure 0 queues_number 12 queues_size 256
> flow configure 1 queues_number 12 queues_size 256
> flow configure 2 queues_number 12 queues_size 256
> port start all
> start
> 
> set raw_encap 0 eth dst is 00:16:3e:52:bd:37 src is 00:16:3e:6e:16:e0
> type is 2048 has_vlan is 0 / ipv4 src is 110.240.52.255 dst is
> 189.68.183.147 proto is 17 fragment_offset is 0 packet_id is 1 tos is
> 102 ttl is 189 version_ihl is 69 / udp src is 56800 dst is 4789 / vxlan
> vni is 3 / end_set
> set sample_actions 0 represented_port ethdev_port_id 0 / end
> 
> flow indirect_action 0 create action_id 5 transfer list actions sample
> ratio 1 index 0 / represented_port ethdev_port_id 2 / end
> flow actions_template 0 create transfer actions_template_id 6 template
> indirect_list handle 5 / end mask  indirect_list handle 5 / end
> 
> Result:
> *** stack smashing detected ***: terminated
> 
> The corruption occurred in `parse_int()` called from
> `parse_indlst_id2ptr()`.
> 
> Inside `parse_int()` the arg parameter referenced 8 bytes of memory
> while the target buffer was 4 bytes allocated on caller optimized stack:
> 
> (gdb) p *arg
> $1 = { ... size = 8, ...}
> 

Thanks Gregory, I can see the problem now.

if 'handle' is not an address, but an ID, your V3 makes sense to convert
its type to 'uint32_t', and it solves to problem. Same for 'conf'.


BUT back to the root cause of the problem,
`parse_int()` tries to be generic and it support different size of
variables [1], but it fails on this.

`parse_int()` gets 'size' as argument, but it doesn't use parameter
value, instead overwrites it with 'size = arg->size;' and uses this
value, in this case when context provides larger variable size than what
`parse_int()` gets as parameter, the problem you observed occurs.

What do you think to use 'size' from parameter list, as it is intended,
instead of using 'arg->size'?
Or perhaps use 'buf' and 'size' from parameter if they are valid, else
get the from context/arg [2]?
I think this solves your problem, can you please verify it?

btw, 'buf' usage is a little more complex, since `parse_int()` checks
for "ctx->object != NULL" to continue, I can't really be sure about
intention there, but please check usage in `parse_port()`, it looks like
when 'buf' provided expectation is to get parsed value in the 'buf'.




[1]
  switch (size) {
  case sizeof(uint8_t):
    ...
  case sizeof(uint16_t):
    ...
  ...
  case sizeof(uint64_t):
    ...


[2]
 diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
 index 0d521159e97d..cd123c69265e 100644
 --- a/app/test-pmd/cmdline_flow.c
 +++ b/app/test-pmd/cmdline_flow.c
 @@ -10805,8 +10805,10 @@ parse_int(struct context *ctx, const struct
token *token,
                         goto error;
                 return len;
         }
 -       buf = (uint8_t *)ctx->object + arg->offset;
 -       size = arg->size;
 +       if (buf == NULL || size == 0) {
 +               buf = (uint8_t *)ctx->object + arg->offset;
 +               size = arg->size;
 +       }
         if (u > RTE_LEN2MASK(size * CHAR_BIT, uint64_t))
                 return -1;
  objmask:



> 
>>>> Inside 'parse_indlst_id2ptr()',
>>>> 'parse_int()' can work or 32bits and 64bits variables, so that one
>>>> is OK.
>>>> But both 'port_action_handle_get_by_id()' &
>>>> 'indirect_action_list_conf_get()' gets 'id' as parameter and they get
>>>> 32bits argument, when 'id' is 64bit won't it will be cast to 32bits and
>>>> loose data, should those functions needs to be updated as well.
>>>>
>>
>> Can you please reply to above question, about changing 'id' type impact
>> to other functions using it?
>>
> 
> I've missed that.
> Need to re-think.
> 
> Regards,
> Gregory
> 
>>
>>>>
>>>>
>>>>> The patch updated variable size for translation results.
>>>>>
>>>>> Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
>>>>> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
>>>>> ---
>>>>>  app/test-pmd/cmdline_flow.c | 5 +++--
>>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>>>>> index 0d521159e9..cf1ca33208 100644
>>>>> --- a/app/test-pmd/cmdline_flow.c
>>>>> +++ b/app/test-pmd/cmdline_flow.c
>>>>> @@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx,
>>>>> const struct token *token,
>>>>>       struct rte_flow_action *action = ctx->object;
>>>>>       struct rte_flow_action_indirect_list *action_conf;
>>>>>       const struct indlst_conf *indlst_conf;
>>>>> -     uint32_t id;
>>>>> +     uint64_t id;
>>>>>       int ret;
>>>>>
>>>>>       if (!action)
>>>>> @@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx,
>>>>> const struct token *token,
>>>>>       action_conf->handle = (typeof(action_conf->handle))
>>>>>                               port_action_handle_get_by_id(ctx->port,
>>>>> id);
>>>>>               if (!action_conf->handle) {
>>>>> -                     printf("no indirect list handle for id %u\n",
>>>>> id);
>>>>> +                     printf("no indirect list handle for id
>>>>> %"PRIu64"\n",
>>>>> +                            id);
>>>>>                       return -1;
>>>>>               }
>>>>>               break;
>>>>
>>>>
>>
>>

Re: [PATCH] app/testpmd: fix indirect action list parameters parsing

[Etelson, Gregory]

Hello Ferruh,

[:snip:]

>> *** stack smashing detected ***: terminated
>>
>> The corruption occurred in `parse_int()` called from
>> `parse_indlst_id2ptr()`.
>>
>> Inside `parse_int()` the arg parameter referenced 8 bytes of memory
>> while the target buffer was 4 bytes allocated on caller optimized stack:
>>
>> (gdb) p *arg
>> $1 = { ... size = 8, ...}
>>
>
> Thanks Gregory, I can see the problem now.
>

[: Thumbs up :]

[:snip:]

> BUT back to the root cause of the problem,
> `parse_int()` tries to be generic and it support different size of
> variables [1], but it fails on this.
>
> `parse_int()` gets 'size' as argument, but it doesn't use parameter
> value, instead overwrites it with 'size = arg->size;' and uses this
> value, in this case when context provides larger variable size than what
> `parse_int()` gets as parameter, the problem you observed occurs.
>
> What do you think to use 'size' from parameter list, as it is intended,
> instead of using 'arg->size'?
> Or perhaps use 'buf' and 'size' from parameter if they are valid, else
> get the from context/arg [2]?
> I think this solves your problem, can you please verify it?
>
> btw, 'buf' usage is a little more complex, since `parse_int()` checks
> for "ctx->object != NULL" to continue, I can't really be sure about
> intention there, but please check usage in `parse_port()`, it looks like
> when 'buf' provided expectation is to get parsed value in the 'buf'.
>
>
>
>
> [1]
>  switch (size) {
>  case sizeof(uint8_t):
>    ...
>  case sizeof(uint16_t):
>    ...
>  ...
>  case sizeof(uint64_t):
>    ...
>
>
> [2]
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 0d521159e97d..cd123c69265e 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -10805,8 +10805,10 @@ parse_int(struct context *ctx, const struct
> token *token,
>                         goto error;
>                 return len;
>         }
> -       buf = (uint8_t *)ctx->object + arg->offset;
> -       size = arg->size;
> +       if (buf == NULL || size == 0) {
> +               buf = (uint8_t *)ctx->object + arg->offset;
> +               size = arg->size;
> +       }
>         if (u > RTE_LEN2MASK(size * CHAR_BIT, uint64_t))
>                 return -1;
>  objmask:
>

About the fault root cause.
There were 2 uncoupled resources in that case: static token size and 
variable size passed to parse_int().
parse_int() caller must provide a buffer large enough for token size.
Otherwise parse_int() will corrupt memory outside the input buffer.

In the generic solution parse_int() caller allocates target buffer using 
existing knowledge about input token size.

Testpmd add_port() imitates the ARGS_ENTRY() macro that extrapolates 
target buffer size from RTE structure member.

Current testpmd cannot use that approach directly because indirect action 
references internal testpmd ID.

Testpmd indirect ID has no defined type or token that leads to indirect ID 
parser.

As a solution, testpmd can provide centralized parser function for all 
indirect IDs. The function will parse ID value and use the token as the 
key to indirect database search:

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index ce71818705..7fb3b61d37 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c

+typedef uint32_t indirect_id_t;
+
+static int
+parse_indirect_index(struct context *ctx, const struct token *token,
+               const char *str, unsigned int len,
+               void *buf, unsigned int size)
+{
+       indirect_id_t id;
+
+       ctx->object = &id;
+       parse_int(ctx, token, str, len, ctx->object, sizeof(id));
+
+       switch (ctx->curr) {
+       case INDIRECT_ACTION_ID2PTR:
+               /**/
+               break;
+       case INDIRECT_LIST_ACTION_ID2PTR_HANDLE:
+               /**/
+               break;
+       case INDIRECT_LIST_ACTION_ID2PTR_CONF:
+               /**/
+               break;
+       default:
+               /**/
+       }
+
+
+}
+

Regards,
Gregory

Re: [PATCH] app/testpmd: fix indirect action list parameters parsing

[Ferruh Yigit]

On 11/10/2023 5:41 PM, Etelson, Gregory wrote:
> Hello Ferruh,
> 
> [:snip:]
> 
>>> *** stack smashing detected ***: terminated
>>>
>>> The corruption occurred in `parse_int()` called from
>>> `parse_indlst_id2ptr()`.
>>>
>>> Inside `parse_int()` the arg parameter referenced 8 bytes of memory
>>> while the target buffer was 4 bytes allocated on caller optimized stack:
>>>
>>> (gdb) p *arg
>>> $1 = { ... size = 8, ...}
>>>
>>
>> Thanks Gregory, I can see the problem now.
>>
> 
> [: Thumbs up :]
> 
> [:snip:]
> 
>> BUT back to the root cause of the problem,
>> `parse_int()` tries to be generic and it support different size of
>> variables [1], but it fails on this.
>>
>> `parse_int()` gets 'size' as argument, but it doesn't use parameter
>> value, instead overwrites it with 'size = arg->size;' and uses this
>> value, in this case when context provides larger variable size than what
>> `parse_int()` gets as parameter, the problem you observed occurs.
>>
>> What do you think to use 'size' from parameter list, as it is intended,
>> instead of using 'arg->size'?
>> Or perhaps use 'buf' and 'size' from parameter if they are valid, else
>> get the from context/arg [2]?
>> I think this solves your problem, can you please verify it?
>>
>> btw, 'buf' usage is a little more complex, since `parse_int()` checks
>> for "ctx->object != NULL" to continue, I can't really be sure about
>> intention there, but please check usage in `parse_port()`, it looks like
>> when 'buf' provided expectation is to get parsed value in the 'buf'.
>>
>>
>>
>>
>> [1]
>>  switch (size) {
>>  case sizeof(uint8_t):
>>    ...
>>  case sizeof(uint16_t):
>>    ...
>>  ...
>>  case sizeof(uint64_t):
>>    ...
>>
>>
>> [2]
>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>> index 0d521159e97d..cd123c69265e 100644
>> --- a/app/test-pmd/cmdline_flow.c
>> +++ b/app/test-pmd/cmdline_flow.c
>> @@ -10805,8 +10805,10 @@ parse_int(struct context *ctx, const struct
>> token *token,
>>                         goto error;
>>                 return len;
>>         }
>> -       buf = (uint8_t *)ctx->object + arg->offset;
>> -       size = arg->size;
>> +       if (buf == NULL || size == 0) {
>> +               buf = (uint8_t *)ctx->object + arg->offset;
>> +               size = arg->size;
>> +       }
>>         if (u > RTE_LEN2MASK(size * CHAR_BIT, uint64_t))
>>                 return -1;
>>  objmask:
>>
> 
> About the fault root cause.
> There were 2 uncoupled resources in that case: static token size and
> variable size passed to parse_int().
> parse_int() caller must provide a buffer large enough for token size.
> Otherwise parse_int() will corrupt memory outside the input buffer.
> 

As you said 'parse_int()' has two sizes, 'token->size' and 'size'
function argument. Why function ignores 'size' argument and only uses
'token->size', I think this is a mistake.


If 'parse_int()' doesn't use 'buf' and 'size' arguments at all, why it
has them?


> In the generic solution parse_int() caller allocates target buffer using
> existing knowledge about input token size.
> 
> Testpmd add_port() imitates the ARGS_ENTRY() macro that extrapolates
> target buffer size from RTE structure member.
> 
> Current testpmd cannot use that approach directly because indirect
> action references internal testpmd ID.
> 
> Testpmd indirect ID has no defined type or token that leads to indirect
> ID parser.
> 
> As a solution, testpmd can provide centralized parser function for all
> indirect IDs. The function will parse ID value and use the token as the
> key to indirect database search:
> 

Although it sounds reasonable to have indirect id parser, won't it have
exact same problem?

If token size if 64 bits as it is now, as far as I can see below code
will have same stack corruption problem.


I think we should update parse_int function, to use either function
parameters or context values, but changes has potential side effect and
timing is not good for it, let's continue with your v3 for now.


> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index ce71818705..7fb3b61d37 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> 
> +typedef uint32_t indirect_id_t;
> +
> +static int
> +parse_indirect_index(struct context *ctx, const struct token *token,
> +               const char *str, unsigned int len,
> +               void *buf, unsigned int size)
> +{
> +       indirect_id_t id;
> +
> +       ctx->object = &id;
> +       parse_int(ctx, token, str, len, ctx->object, sizeof(id));
> +
> +       switch (ctx->curr) {
> +       case INDIRECT_ACTION_ID2PTR:
> +               /**/
> +               break;
> +       case INDIRECT_LIST_ACTION_ID2PTR_HANDLE:
> +               /**/
> +               break;
> +       case INDIRECT_LIST_ACTION_ID2PTR_CONF:
> +               /**/
> +               break;
> +       default:
> +               /**/
> +       }
> +
> +
> +}
> +
> 
> Regards,
> Gregory

Re: [PATCH] app/testpmd: fix indirect action list parameters parsing

[Etelson, Gregory]

Hello Ferruh,

>> About the fault root cause.
>> There were 2 uncoupled resources in that case: static token size and
>> variable size passed to parse_int().
>> parse_int() caller must provide a buffer large enough for token size.
>> Otherwise parse_int() will corrupt memory outside the input buffer.
>>
>
> As you said 'parse_int()' has two sizes, 'token->size' and 'size'
> function argument. Why function ignores 'size' argument and only uses
> 'token->size', I think this is a mistake.
>
>
> If 'parse_int()' doesn't use 'buf' and 'size' arguments at all, why it
> has them?
>
>

parse_int() receives target buffer size as an argument and also has access 
to original token size through the arg pointer.
In that configuration the token size has priority.

parse_int() must compare token and buffer sizes and return an error if 
target buffer size did not fit.

That will not break existing parser_int() functionality.
I'll send a fix in a new patch series.

>> In the generic solution parse_int() caller allocates target buffer using
>> existing knowledge about input token size.
>>
>> Testpmd add_port() imitates the ARGS_ENTRY() macro that extrapolates
>> target buffer size from RTE structure member.
>>
>> Current testpmd cannot use that approach directly because indirect
>> action references internal testpmd ID.
>>
>> Testpmd indirect ID has no defined type or token that leads to indirect
>> ID parser.
>>
>> As a solution, testpmd can provide centralized parser function for all
>> indirect IDs. The function will parse ID value and use the token as the
>> key to indirect database search:
>>
>
> Although it sounds reasonable to have indirect id parser, won't it have
> exact same problem?
>
> If token size if 64 bits as it is now, as far as I can see below code
> will have same stack corruption problem.
>

The function parses indirect ID. That ID is translated into the original 
object of any size according to token type. That means testpmd will have 
common pool of 32 bits IDs for all possible indirect translations.

>
> I think we should update parse_int function, to use either function
> parameters or context values, but changes has potential side effect and
> timing is not good for it, let's continue with your v3 for now.
>
>

[: Thumbs up :]

Regards,
Gregory

[PATCH v2] app/testpmd: fix indirect action list parameters parsing

[Gregory Etelson]

Indirect actions list arguments parser was configured to place target
number into 64bit value, while the code provided 32bits memory.

The patch updated variable size for translation results.

Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
v2: define `id` as uintptr_t
---
 app/test-pmd/cmdline_flow.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 0d521159e9..397f9bc3eb 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
 	struct rte_flow_action *action = ctx->object;
 	struct rte_flow_action_indirect_list *action_conf;
 	const struct indlst_conf *indlst_conf;
-	uint32_t id;
+	uintptr_t id;
 	int ret;
 
 	if (!action)
@@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
 	action_conf->handle = (typeof(action_conf->handle))
 				port_action_handle_get_by_id(ctx->port, id);
 		if (!action_conf->handle) {
-			printf("no indirect list handle for id %u\n", id);
+			printf("no indirect list handle for id %"PRIu64"\n",
+			       id);
 			return -1;
 		}
 		break;
-- 
2.39.2

Re: [PATCH v2] app/testpmd: fix indirect action list parameters parsing

[Stephen Hemminger]

On Thu, 9 Nov 2023 20:36:37 +0200
Gregory Etelson <getelson@nvidia.com> wrote:

> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 0d521159e9..397f9bc3eb 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
>  	struct rte_flow_action *action = ctx->object;
>  	struct rte_flow_action_indirect_list *action_conf;
>  	const struct indlst_conf *indlst_conf;
> -	uint32_t id;
> +	uintptr_t id;
>  	int ret;
>  
>  	if (!action)
> @@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
>  	action_conf->handle = (typeof(action_conf->handle))
>  				port_action_handle_get_by_id(ctx->port, id);
>  		if (!action_conf->handle) {
> -			printf("no indirect list handle for id %u\n", id);
> +			printf("no indirect list handle for id %"PRIu64"\n",
> +			       id);

On 32 bit platforms uintptr_t is 32 bits.
Uintptr_t is always a typedef for unsigned long so use %lu here instead.

Re: [PATCH v2] app/testpmd: fix indirect action list parameters parsing

[Bruce Richardson]

On Thu, Nov 09, 2023 at 11:41:37AM -0800, Stephen Hemminger wrote:
> On Thu, 9 Nov 2023 20:36:37 +0200
> Gregory Etelson <getelson@nvidia.com> wrote:
> 
> > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > index 0d521159e9..397f9bc3eb 100644
> > --- a/app/test-pmd/cmdline_flow.c
> > +++ b/app/test-pmd/cmdline_flow.c
> > @@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
> >  	struct rte_flow_action *action = ctx->object;
> >  	struct rte_flow_action_indirect_list *action_conf;
> >  	const struct indlst_conf *indlst_conf;
> > -	uint32_t id;
> > +	uintptr_t id;
> >  	int ret;
> >  
> >  	if (!action)
> > @@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
> >  	action_conf->handle = (typeof(action_conf->handle))
> >  				port_action_handle_get_by_id(ctx->port, id);
> >  		if (!action_conf->handle) {
> > -			printf("no indirect list handle for id %u\n", id);
> > +			printf("no indirect list handle for id %"PRIu64"\n",
> > +			       id);
> 
> On 32 bit platforms uintptr_t is 32 bits.
> Uintptr_t is always a typedef for unsigned long so use %lu here instead.

PRIuPTR(and PRIxPTR) is the corresponding define from inttypes.h.

/Bruce

[PATCH v3] app/testpmd: fix indirect action list ID size

[Gregory Etelson]

Testpmd maps indirect action lists handlers into application IDs.
Testpmd API defines indirect IDs as 32 bits values.

The patch fixes indirect IDs size in  ACTION_INDIRECT_LIST_HANDLE
and ACTION_INDIRECT_LIST_CONF tokens

Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
v2: define `id` as uintptr_t
v3: define indirect ID size as 32 bits
---
 app/test-pmd/cmdline_flow.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 0d521159e9..ce71818705 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -7383,13 +7383,13 @@ static const struct token token_list[] = {
 		.name = "handle",
 		.help = "indirect list handle",
 		.next = NEXT(next_ial, NEXT_ENTRY(INDIRECT_LIST_ACTION_ID2PTR_HANDLE)),
-		.args = ARGS(ARGS_ENTRY_ARB(0, sizeof(uintptr_t))),
+		.args = ARGS(ARGS_ENTRY_ARB(0, sizeof(uint32_t))),
 	},
 	[ACTION_INDIRECT_LIST_CONF] = {
 		.name = "conf",
 		.help = "indirect list configuration",
 		.next = NEXT(next_ial, NEXT_ENTRY(INDIRECT_LIST_ACTION_ID2PTR_CONF)),
-		.args = ARGS(ARGS_ENTRY_ARB(0, sizeof(uintptr_t))),
+		.args = ARGS(ARGS_ENTRY_ARB(0, sizeof(uint32_t))),
 	},
 	[INDIRECT_LIST_ACTION_ID2PTR_HANDLE] = {
 		.type = "UNSIGNED",
-- 
2.39.2

Re: [PATCH v3] app/testpmd: fix indirect action list ID size

[Ferruh Yigit]

On 11/10/2023 7:04 AM, Gregory Etelson wrote:
> Testpmd maps indirect action lists handlers into application IDs.
> Testpmd API defines indirect IDs as 32 bits values.
> 
> The patch fixes indirect IDs size in  ACTION_INDIRECT_LIST_HANDLE
> and ACTION_INDIRECT_LIST_CONF tokens
> 
> Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
> Cc: stable@dpdk.org
>
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.