[PATCH iproute2 0/2] two fixes for tc ct command

[PATCH iproute2 0/2] two fixes for tc ct command

[Roi Dayan]

Hi,

The first first is that ct commit nat src/dst should accept
the command even without an address and the driver fills
the address from the tuple.
The second commit fixes a segmentaion fault reading invalid
argv pointer which is passing the available args passed by
the user.

Thanks,
Roi


Roi Dayan (2):
  tc: ct: Fix ct commit nat forcing addr
  tc: ct: Fix invalid pointer derefence

 man/man8/tc-ct.8 | 2 +-
 tc/m_ct.c        | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.38.0

[PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr

[Roi Dayan]

Action ct commit should accept nat src/dst without an addr. Fix it.

Fixes: c8a494314c40 ("tc: Introduce tc ct action")
Signed-off-by: Roi Dayan <roid@nvidia.com>
Reviewed-by: Paul Blakey <paulb@nvidia.com>
---
 man/man8/tc-ct.8 | 2 +-
 tc/m_ct.c        | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8
index 2fb81ca29aa4..78d05e430c36 100644
--- a/man/man8/tc-ct.8
+++ b/man/man8/tc-ct.8
@@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit).
 Specify a masked 128bit label to set for the connection (only valid with commit).
 .TP
 .BI nat " NAT_SPEC"
-.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]"
+.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]"
 
 Specify src/dst and range of nat to configure for the connection (only valid with commit).
 .RS
diff --git a/tc/m_ct.c b/tc/m_ct.c
index a02bf0cc1655..1b8984075a67 100644
--- a/tc/m_ct.c
+++ b/tc/m_ct.c
@@ -23,7 +23,7 @@ usage(void)
 		"	ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n"
 		"	ct [nat] [zone ZONE]\n"
 		"Where: ZONE is the conntrack zone table number\n"
-		"	NAT_SPEC is {src|dst} addr addr1[-addr2] [port port1[-port2]]\n"
+		"	NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n"
 		"\n");
 	exit(-1);
 }
@@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
 
 			NEXT_ARG();
 			if (matches(*argv, "addr") != 0)
-				usage();
+				continue;
 
 			NEXT_ARG();
 			ret = ct_parse_nat_addr_range(*argv, n);
-- 
2.38.0

[PATCH iproute2 2/2] tc: ct: Fix invalid pointer derefence

[Roi Dayan]

Using mcaro NEXT_ARG_FWD does not validate argc.
Use macro NEXT_ARG which validates argc while parsing args
in the same loop iteration.

Fixes: c8a494314c40 ("tc: Introduce tc ct action")
Signed-off-by: Roi Dayan <roid@nvidia.com>
Reviewed-by: Paul Blakey <paulb@nvidia.com>
---
 tc/m_ct.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tc/m_ct.c b/tc/m_ct.c
index 1b8984075a67..e1c4b822f8ac 100644
--- a/tc/m_ct.c
+++ b/tc/m_ct.c
@@ -243,7 +243,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
 				return -1;
 			}
 
-			NEXT_ARG_FWD();
+			NEXT_ARG();
 			if (matches(*argv, "port") != 0)
 				continue;
 
-- 
2.38.0

Re: [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr

[Stephen Hemminger]

On Wed, 16 Nov 2022 09:33:11 +0200
Roi Dayan <roid@nvidia.com> wrote:

> Action ct commit should accept nat src/dst without an addr. Fix it.
> 
> Fixes: c8a494314c40 ("tc: Introduce tc ct action")
> Signed-off-by: Roi Dayan <roid@nvidia.com>
> Reviewed-by: Paul Blakey <paulb@nvidia.com>
> ---
>  man/man8/tc-ct.8 | 2 +-
>  tc/m_ct.c        | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8
> index 2fb81ca29aa4..78d05e430c36 100644
> --- a/man/man8/tc-ct.8
> +++ b/man/man8/tc-ct.8
> @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit).
>  Specify a masked 128bit label to set for the connection (only valid with commit).
>  .TP
>  .BI nat " NAT_SPEC"
> -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]"
> +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]"
>  
>  Specify src/dst and range of nat to configure for the connection (only valid with commit).
>  .RS
> diff --git a/tc/m_ct.c b/tc/m_ct.c
> index a02bf0cc1655..1b8984075a67 100644
> --- a/tc/m_ct.c
> +++ b/tc/m_ct.c
> @@ -23,7 +23,7 @@ usage(void)
>  		"	ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n"
>  		"	ct [nat] [zone ZONE]\n"
>  		"Where: ZONE is the conntrack zone table number\n"
> -		"	NAT_SPEC is {src|dst} addr addr1[-addr2] [port port1[-port2]]\n"
> +		"	NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n"
>  		"\n");
>  	exit(-1);
>  }
> @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
>  
>  			NEXT_ARG();
>  			if (matches(*argv, "addr") != 0)
> -				usage();
> +				continue;
>  

This confuses me. Doing continue here will cause the current argument to be reprocessed so
it would expect it to be zone | nat | clear | commit | force | index | mark | label
which is not right.

Re: [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr

[Roi Dayan]

On 16/11/2022 20:21, Stephen Hemminger wrote:
> On Wed, 16 Nov 2022 09:33:11 +0200
> Roi Dayan <roid@nvidia.com> wrote:
> 
>> Action ct commit should accept nat src/dst without an addr. Fix it.
>>
>> Fixes: c8a494314c40 ("tc: Introduce tc ct action")
>> Signed-off-by: Roi Dayan <roid@nvidia.com>
>> Reviewed-by: Paul Blakey <paulb@nvidia.com>
>> ---
>>  man/man8/tc-ct.8 | 2 +-
>>  tc/m_ct.c        | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8
>> index 2fb81ca29aa4..78d05e430c36 100644
>> --- a/man/man8/tc-ct.8
>> +++ b/man/man8/tc-ct.8
>> @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit).
>>  Specify a masked 128bit label to set for the connection (only valid with commit).
>>  .TP
>>  .BI nat " NAT_SPEC"
>> -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]"
>> +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]"
>>  
>>  Specify src/dst and range of nat to configure for the connection (only valid with commit).
>>  .RS
>> diff --git a/tc/m_ct.c b/tc/m_ct.c
>> index a02bf0cc1655..1b8984075a67 100644
>> --- a/tc/m_ct.c
>> +++ b/tc/m_ct.c
>> @@ -23,7 +23,7 @@ usage(void)
>>  		"	ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n"
>>  		"	ct [nat] [zone ZONE]\n"
>>  		"Where: ZONE is the conntrack zone table number\n"
>> -		"	NAT_SPEC is {src|dst} addr addr1[-addr2] [port port1[-port2]]\n"
>> +		"	NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n"
>>  		"\n");
>>  	exit(-1);
>>  }
>> @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
>>  
>>  			NEXT_ARG();
>>  			if (matches(*argv, "addr") != 0)
>> -				usage();
>> +				continue;
>>  
> 
> This confuses me. Doing continue here will cause the current argument to be reprocessed so
> it would expect it to be zone | nat | clear | commit | force | index | mark | label
> which is not right.
> 
> 

its the opposite. "nat" came first. if matches() didn't find "addr"
it continues the loop of args. if matches did find "addr" it continues
to next line which is ct_parse_nat_addr_range() to parse the address.

Re: [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr

[Roi Dayan]

On 17/11/2022 7:35, Roi Dayan wrote:
> 
> 
> On 16/11/2022 20:21, Stephen Hemminger wrote:
>> On Wed, 16 Nov 2022 09:33:11 +0200
>> Roi Dayan <roid@nvidia.com> wrote:
>>
>>> Action ct commit should accept nat src/dst without an addr. Fix it.
>>>
>>> Fixes: c8a494314c40 ("tc: Introduce tc ct action")
>>> Signed-off-by: Roi Dayan <roid@nvidia.com>
>>> Reviewed-by: Paul Blakey <paulb@nvidia.com>
>>> ---
>>>  man/man8/tc-ct.8 | 2 +-
>>>  tc/m_ct.c        | 4 ++--
>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8
>>> index 2fb81ca29aa4..78d05e430c36 100644
>>> --- a/man/man8/tc-ct.8
>>> +++ b/man/man8/tc-ct.8
>>> @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit).
>>>  Specify a masked 128bit label to set for the connection (only valid with commit).
>>>  .TP
>>>  .BI nat " NAT_SPEC"
>>> -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]"
>>> +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]"
>>>  
>>>  Specify src/dst and range of nat to configure for the connection (only valid with commit).
>>>  .RS
>>> diff --git a/tc/m_ct.c b/tc/m_ct.c
>>> index a02bf0cc1655..1b8984075a67 100644
>>> --- a/tc/m_ct.c
>>> +++ b/tc/m_ct.c
>>> @@ -23,7 +23,7 @@ usage(void)
>>>  		"	ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n"
>>>  		"	ct [nat] [zone ZONE]\n"
>>>  		"Where: ZONE is the conntrack zone table number\n"
>>> -		"	NAT_SPEC is {src|dst} addr addr1[-addr2] [port port1[-port2]]\n"
>>> +		"	NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n"
>>>  		"\n");
>>>  	exit(-1);
>>>  }
>>> @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
>>>  
>>>  			NEXT_ARG();
>>>  			if (matches(*argv, "addr") != 0)
>>> -				usage();
>>> +				continue;
>>>  
>>
>> This confuses me. Doing continue here will cause the current argument to be reprocessed so
>> it would expect it to be zone | nat | clear | commit | force | index | mark | label
>> which is not right.
>>
>>
> 
> its the opposite. "nat" came first. if matches() didn't find "addr"
> it continues the loop of args. if matches did find "addr" it continues
> to next line which is ct_parse_nat_addr_range() to parse the address.
> 
> 

Got your comment wrong so yes the current arg will be reprocessed
and this is what we want.
This will make "addr" optional and there should be some action
after ct commit nat. next loop iteration should break and
continue parse next action usually a goto action.

Re: [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr

[Roi Dayan]

On 17/11/2022 16:00, Roi Dayan wrote:
> 
> 
> On 17/11/2022 7:35, Roi Dayan wrote:
>>
>>
>> On 16/11/2022 20:21, Stephen Hemminger wrote:
>>> On Wed, 16 Nov 2022 09:33:11 +0200
>>> Roi Dayan <roid@nvidia.com> wrote:
>>>
>>>> Action ct commit should accept nat src/dst without an addr. Fix it.
>>>>
>>>> Fixes: c8a494314c40 ("tc: Introduce tc ct action")
>>>> Signed-off-by: Roi Dayan <roid@nvidia.com>
>>>> Reviewed-by: Paul Blakey <paulb@nvidia.com>
>>>> ---
>>>>  man/man8/tc-ct.8 | 2 +-
>>>>  tc/m_ct.c        | 4 ++--
>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8
>>>> index 2fb81ca29aa4..78d05e430c36 100644
>>>> --- a/man/man8/tc-ct.8
>>>> +++ b/man/man8/tc-ct.8
>>>> @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit).
>>>>  Specify a masked 128bit label to set for the connection (only valid with commit).
>>>>  .TP
>>>>  .BI nat " NAT_SPEC"
>>>> -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]"
>>>> +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]"
>>>>  
>>>>  Specify src/dst and range of nat to configure for the connection (only valid with commit).
>>>>  .RS
>>>> diff --git a/tc/m_ct.c b/tc/m_ct.c
>>>> index a02bf0cc1655..1b8984075a67 100644
>>>> --- a/tc/m_ct.c
>>>> +++ b/tc/m_ct.c
>>>> @@ -23,7 +23,7 @@ usage(void)
>>>>  		"	ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n"
>>>>  		"	ct [nat] [zone ZONE]\n"
>>>>  		"Where: ZONE is the conntrack zone table number\n"
>>>> -		"	NAT_SPEC is {src|dst} addr addr1[-addr2] [port port1[-port2]]\n"
>>>> +		"	NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n"
>>>>  		"\n");
>>>>  	exit(-1);
>>>>  }
>>>> @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
>>>>  
>>>>  			NEXT_ARG();
>>>>  			if (matches(*argv, "addr") != 0)
>>>> -				usage();
>>>> +				continue;
>>>>  
>>>
>>> This confuses me. Doing continue here will cause the current argument to be reprocessed so
>>> it would expect it to be zone | nat | clear | commit | force | index | mark | label
>>> which is not right.
>>>
>>>
>>
>> its the opposite. "nat" came first. if matches() didn't find "addr"
>> it continues the loop of args. if matches did find "addr" it continues
>> to next line which is ct_parse_nat_addr_range() to parse the address.
>>
>>
> 
> Got your comment wrong so yes the current arg will be reprocessed
> and this is what we want.
> This will make "addr" optional and there should be some action
> after ct commit nat. next loop iteration should break and
> continue parse next action usually a goto action.
> 


Hi Stephen,

Can you look at this again please?
this is the same as done in other args like "port" right after
and probably in other actions.
If "addr" is not the next arg we continue the loop and parse again
as expected for the other ct args or getting to else and breaking
to continue.

Thanks,
Roi