On 2/9/2024 2:01 PM, Gregory Etelson wrote: > Hello Ferruh, > > The patch did not fix an existing issue. >
I remember it is for protection against similar buffer overwrite issue, not exactly for that specific issue. > It was proposed the code improvement. > Let's drop it for now. > Agree, I will drop. > I'll post a general solution in the next iteration. > Ack, thanks. > Regards, > Gregory > ------------------------------------------------------------------------ > *From:* Ferruh Yigit <[email protected]> > *Sent:* Friday, February 9, 2024 15:55 > *To:* Dariusz Sosnowski <[email protected]>; Gregory Etelson > <[email protected]>; [email protected] <[email protected]> > *Cc:* Maayan Kashani <[email protected]>; Ori Kam <[email protected]>; > Aman Singh <[email protected]>; Yuying Zhang > <[email protected]> > *Subject:* Re: [PATCH] app/testpmd: add size parameter to raw_encap action > > External email: Use caution opening links or attachments > > > On 2/9/2024 1:43 PM, Dariusz Sosnowski wrote: >>> -----Original Message----- >>> From: Dariusz Sosnowski >>> Sent: Friday, February 9, 2024 12:04 >>> To: Gregory Etelson <[email protected]>; [email protected] >>> Cc: Gregory Etelson <[email protected]>; Maayan Kashani >>> <[email protected]>; Ori Kam <[email protected]>; Aman Singh >>> <[email protected]>; Yuying Zhang <[email protected]> >>> Subject: RE: [PATCH] app/testpmd: add size parameter to raw_encap action >>> >>> Hi Gregory, >>> >>>> -----Original Message----- >>>> From: Gregory Etelson <[email protected]> >>>> Sent: Thursday, October 26, 2023 09:31 >>>> To: [email protected] >>>> Cc: Gregory Etelson <[email protected]>; Maayan Kashani >>>> <[email protected]>; Ori Kam <[email protected]>; Aman Singh >>>> <[email protected]>; Yuying Zhang <[email protected]> >>>> Subject: [PATCH] app/testpmd: add size parameter to raw_encap action >>>> >>>> Testpmd always provides RAW_ENCAP flow action configuration with encap >>>> buffer and the buffer size. >>>> That implementation does not allow to create non-masked raw_encap >>>> action in the template API actions template. >>>> >>>> The patch adds the `size` parameter to testpmd `raw_encap` action >>>> configuration. >>>> Testpmd can create non-masked raw-encap action template and specify >>>> encap buffer during flow creation. >>>> >>>> Example: >>>> >>>> # total data size is 50 >>>> testpmd> set raw_encap 0 \ >>>> eth src is 11:22:33:44:55:66 dst is aa:bb:cc:dd:01:aa / \ >>>> ipv4 src is 31.31.31.31 dst is 63.63.63.1 / udp src is 1 / \ >>>> vxlan vni is 1 / end_set >>>> >>>> testpmd> flow actions_template 0 create ingress \ >>>> actions_template_id 50 \ >>>> template raw_encap size 50 / jump / end \ >>>> mask raw_encap size 50 / jump / end \ >>>> >>>> tstpmd> flow queue 0 create 0 template_table 0 \ >>>> pattern_template 0 actions_template 0 postpone no \ >>>> pattern ... end \ >>>> actions raw_encap index 0 / jump group 1 / end >>>> >>>> The new `size` parameter is mutually exclusive with the existing >>>> `index` parameter. >>>> >>>> Signed-off-by: Gregory Etelson <[email protected]> >>> >>> The following sequence of commands results in "Bad arguments" error, but I >>> think it should be accepted. >>> >>> testpmd> port stop all >>> Stopping ports... >>> Checking link statuses... >>> Done >>> testpmd> flow configure 0 queues_number 4 queues_size 64 >>> Configure flows on port 0: number of queues 4 with 64 elements >>> testpmd> port start all >>> Port 0: B8:CE:F6:7B:D8:E0 >>> Checking link statuses... >>> Done >>> testpmd> set raw_encap 0 eth src is 11:22:33:44:55:66 dst is >>> testpmd> aa:bb:cc:dd:01:aa / ipv4 src is 31.31.31.31 dst is 63.63.63.1 / >>> testpmd> udp src is 1 / vxlan vni is 1 / end_set flow actions_template 0 >>> testpmd> create ingress actions_template_id 100 template raw_encap index >>> testpmd> 0 / jump / end mask raw_encap index 0 / jump / end >>> Bad arguments >> >> I bisected the tree, and it appears that this issue is not caused by this >> commit, but it's an existing problem in testpmd. >> The root cause of "Bad arguments" is the fact that when parsing function for >> raw_encap index is called, >> parse_int() is called with size == 0, but raw_encap index size is >> sizeof(size_t). >> This causes a failure in validation introduced in commit 913b919906da >> ("app/testpmd: add size validation to token parsers"). >> >> The same issue appears for other cases where parse_int() is called with size >> == 0 e.g., parsing RSS queues in RSS flow action. >> I'll provide a patch for testpmd which addresses that. >> > > Hi Dariusz, > > I merged that patch recently, and it is still not merged to main repo, > so we can drop it from next-net if required, instead of having a fix on > top of it. > > That patch is because of a defect that overwrites some memory, and side > impact you mentioned was not expected. > Can you please sync with Gregory about it first? >
