[PATCH] netfilter: nf_conntrack_sip: fix parsing error

ct_sip_parse_numerical_param can only return 0 or 1, but the caller is
checking parsing error using < 0

Signed-off-by: Tong Zhang <redacted>
---
 net/netfilter/nf_conntrack_sip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index b83dc9bf0a5d..08873694120a 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c

@@ -1269,7 +1269,7 @@ static int process_register_request(struct sk_buff *skb, unsigned int protoff,
 
 	if (ct_sip_parse_numerical_param(ct, *dptr,
 					 matchoff + matchlen, *datalen,
-					 "expires=", NULL, NULL, &expires) < 0) {
+					 "expires=", NULL, NULL, &expires) == 0) {
 		nf_ct_helper_log(skb, ct, "cannot parse expires");
 		return NF_DROP;
 	}

@@ -1375,7 +1375,7 @@ static int process_register_response(struct sk_buff *skb, unsigned int protoff,
 						   matchoff + matchlen,
 						   *datalen, "expires=",
 						   NULL, NULL, &c_expires);
-		if (ret < 0) {
+		if (ret == 0) {
 			nf_ct_helper_log(skb, ct, "cannot parse expires");
 			return NF_DROP;
 		}

-- 
2.25.1

Tong Zhang [off-list ref] wrote:

ct_sip_parse_numerical_param can only return 0 or 1, but the caller is
checking parsing error using < 0

Reviewed-by: Florian Westphal <fw@strlen.de>

On Sat, Aug 15, 2020 at 12:50:30PM -0400, Tong Zhang wrote:

ct_sip_parse_numerical_param can only return 0 or 1, but the caller is
checking parsing error using < 0

Is this are real issue in your setup or probably some static analysis
tool is reporting?

You are right that ct_sip_parse_numerical_param() never returns < 0,
however, looking at:

https://tools.ietf.org/html/rfc3261 see Page 161

expires is optional, my understanding is that your patch is making
this option mandatory.

Hi Pablo,
I'm not an expert in this networking stuff.
But from my point of view there's no point in checking if this
condition is always true.
There's also no need of returning anything from the
ct_sip_parse_numerical_param()
if they are all being ignored like this.

On Fri, Aug 28, 2020 at 2:07 PM Pablo Neira Ayuso [off-list ref] wrote:

Is this are real issue in your setup or probably some static analysis
tool is reporting?

You are right that ct_sip_parse_numerical_param() never returns < 0,
however, looking at:

https://tools.ietf.org/html/rfc3261 see Page 161

expires is optional, my understanding is that your patch is making
this option mandatory.

On Fri, Aug 28, 2020 at 02:14:48PM -0400, Tong Zhang wrote:

Hi Pablo,
I'm not an expert in this networking stuff.
But from my point of view there's no point in checking if this
condition is always true.

Understood.

There's also no need of returning anything from the
ct_sip_parse_numerical_param()
if they are all being ignored like this.

Then probably update this code to ignore the return value?

I think the original code complaining parsing error is there for a reason,
A better way is to modify ct_sip_parse_numerical_param() and let it return
a real parsing error code instead of returning FOUND(1) and NOT FOUND(0)
if deemed necessary
Once again I'm not an expert and I'm may suggest something stupid,
please pardon my ignorance --
- Tong

On Fri, Aug 28, 2020 at 2:19 PM Pablo Neira Ayuso [off-list ref] wrote:

Then probably update this code to ignore the return value?