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

["Etelson, Gregory" <getelson@nvidia.com>]

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