Re: [PATCH] netfilter: nf_conntrack_sip: fix the ct_sip_parse_numerical_param() return value.

[Gavrilov Ilia]

On 4/28/23 22:24, Simon Horman wrote:
> On Wed, Apr 26, 2023 at 03:04:31PM +0000, Gavrilov Ilia wrote:
>> ct_sip_parse_numerical_param() returns only 0 or 1 now.
>> But process_register_request() and process_register_response() imply
>> checking for a negative value if parsing of a numerical header parameter
>> failed. Let's fix it.
>>
>> Found by InfoTeCS on behalf of Linux Verification Center
>> (linuxtesting.org) with SVACE.
>>
>> Fixes: 0f32a40fc91a ("[NETFILTER]: nf_conntrack_sip: create signalling expectations")
>> Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@xxxxxxxxxxx>
> 
> Hi Gavrilov,
> 

Hi Simon, thank you for your answer.

> although it is a slightly unusual convention for kernel code,
> I believe the intention is that this function returns 0 when
> it fails (to parse) and 1 on success. So I think that part is fine.
> 
> What seems a bit broken is the way that callers use the return value.
> 
> 1. The call in process_register_response() looks like this:
> 
> 	ret = ct_sip_parse_numerical_param(...)
> 	if (ret < 0) {
> 		nf_ct_helper_log(skb, ct, "cannot parse expires");
> 		return NF_DROP;
> 	}
> 
>      But ret can only be 0 or 1, so the error handling is never inoked,
>      and a failure to parse is ignored. I guess failure doesn't occur in
>      practice.
> 
>      I suspect this should be:
> 
> 	ret = ct_sip_parse_numerical_param(...)
> 	if (!ret) {
> 		nf_ct_helper_log(skb, ct, "cannot parse expires");
> 		return NF_DROP;
> 	}
> 

ct_sip_parse_numerical_param() returns 0 in to cases 1) when the 
parameter 'expires=' isn't found in the header or 2) it's incorrectly set.
In the first case, the return value should be ignored, since this is a 
normal situation
In the second case, it's better to write to the log and return NF_DROP, 
or ignore it too, then checking the return value can be removed as 
unnecessary.


> 2. The callprocess_register_request() looks like this:
> 
>          if (ct_sip_parse_numerical_param(...)) {
>                  nf_ct_helper_log(skb, ct, "cannot parse expires");
>                  return NF_DROP;
>          }
> 
>     But this seems to treat success as an error and vice versa.
> 
>          if (!ct_sip_parse_numerical_param(...)) {
>                  nf_ct_helper_log(skb, ct, "cannot parse expires");
>                  return NF_DROP;
>          }
> 
>    Or, better:
> 
>          ret = ct_sip_parse_numerical_param(...);
> 	if (!ret) {
> 		...
> 	}
> 

Here is the same as in process_register_response()

ret = ct_sip_parse_numerical_param(...);
if (ret < 0) {
    ...
    return NF_DROP;
}

Maybe it's better to remove the check altogether?



> 
> 3. The invocation in nf_nat_sip() looks like this:
> 
> 	if (ct_sip_parse_numerical_param(...) > 0 &&
> 	    ...)
> 	    ...
> 
>     This seems correct to me.

I agree, everything seems correct here

> 
>> ---
>>   net/netfilter/nf_conntrack_sip.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
>> index 77f5e82d8e3f..d0eac27f6ba0 100644
>> --- a/net/netfilter/nf_conntrack_sip.c
>> +++ b/net/netfilter/nf_conntrack_sip.c
>> @@ -611,7 +611,7 @@ int ct_sip_parse_numerical_param(const struct nf_conn *ct, const char *dptr,
>>   	start += strlen(name);
>>   	*val = simple_strtoul(start, &end, 0);
>>   	if (start == end)
>> -		return 0;
>> +		return -1;
>>   	if (matchoff && matchlen) {
>>   		*matchoff = start - dptr;
>>   		*matchlen = end - start;
>> -- 
>> 2.30.2
>>