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