| Message ID | 20230426150414.2768070-1-Ilia.Gavrilov@infotecs.ru (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netfilter: nf_conntrack_sip: fix the ct_sip_parse_numerical_param() return value. | expand |
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@infotecs.ru> Hi Gavrilov, 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; } 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) { ... } 3. The invocation in nf_nat_sip() looks like this: if (ct_sip_parse_numerical_param(...) > 0 && ...) ... This seems correct to me. > --- > 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 >
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@infotecs.ru> > > 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 >>
On Tue, May 02, 2023 at 11:43:19AM +0000, Gavrilov Ilia wrote: > 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@infotecs.ru> > > > > 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. Sorry, I think I misunderstood the intention of your patch earlier. Do I (now) understand correctly that you are proposing a tristate? a) return 1 if value is found; *val is set b) return 0 if value is not found; *val is unchanged c) return -1 on error; *val is undefined
On 5/2/23 17:05, Simon Horman wrote: > On Tue, May 02, 2023 at 11:43:19AM +0000, Gavrilov Ilia wrote: >> 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@infotecs.ru> >>> >>> 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. > > Sorry, I think I misunderstood the intention of your patch earlier. > > Do I (now) understand correctly that you are proposing a tristate? > > a) return 1 if value is found; *val is set > b) return 0 if value is not found; *val is unchanged > c) return -1 on error; *val is undefined Yes, it seems to me that this was originally intended.
On Tue, May 02, 2023 at 02:16:09PM +0000, Gavrilov Ilia wrote: > On 5/2/23 17:05, Simon Horman wrote: > > On Tue, May 02, 2023 at 11:43:19AM +0000, Gavrilov Ilia wrote: > >> 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@infotecs.ru> > >>> > >>> 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. > > > > Sorry, I think I misunderstood the intention of your patch earlier. > > > > Do I (now) understand correctly that you are proposing a tristate? > > > > a) return 1 if value is found; *val is set > > b) return 0 if value is not found; *val is unchanged > > c) return -1 on error; *val is undefined > > Yes, it seems to me that this was originally intended. Thanks. With my new found understanding, this looks good to me. Reviewed-by: Simon Horman <simon.horman@corigine.com>
On 5/2/23 18:38, Simon Horman wrote: > On Tue, May 02, 2023 at 02:16:09PM +0000, Gavrilov Ilia wrote: >> On 5/2/23 17:05, Simon Horman wrote: >>> On Tue, May 02, 2023 at 11:43:19AM +0000, Gavrilov Ilia wrote: >>>> 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@infotecs.ru> >>>>> >>>>> 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. >>> >>> Sorry, I think I misunderstood the intention of your patch earlier. >>> >>> Do I (now) understand correctly that you are proposing a tristate? >>> >>> a) return 1 if value is found; *val is set >>> b) return 0 if value is not found; *val is unchanged >>> c) return -1 on error; *val is undefined >> >> Yes, it seems to me that this was originally intended. > > Thanks. With my new found understanding, this looks good to me. > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > Hi, Simon. I'm sorry to bother you. Will this patch be applied or rejected? Вest regards, Ilya.
Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru> wrote: > Hi, Simon. > I'm sorry to bother you. > Will this patch be applied or rejected? Please resend, keeping Simons Reviewd-by tag. Please update the commit message as per your and Simons conversation, i.e. that the return value is now a tristate, 0 for not found and -1 for 'malformed' and that you checked all callers that they will do the right thing.
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;
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@infotecs.ru> --- net/netfilter/nf_conntrack_sip.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)