Re: [PATCH] netfilter: nf_conntrack_sip: CSeq 0 is a valid CSeq

[Christophe Leroy]

Le 04/07/2016 Ã 07:48, Liping Zhang a Ãcrit :
> 2016-07-01 17:48 GMT+08:00 Christophe Leroy <christophe.leroy@xxxxxx>:
> > Do not drop packet when CSeq is 0 as 0 is also a valid value for CSeq.
> >
> > --- a/net/netfilter/nf_conntrack_sip.c
> > +++ b/net/netfilter/nf_conntrack_sip.c
> > @@ -1368,6 +1368,7 @@ static int process_sip_response(struct sk_buff *skb, unsigned int protoff,
> >         struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
> >         unsigned int matchoff, matchlen, matchend;
> >         unsigned int code, cseq, i;
> > +       char buf[21];
> >
> >         if (*datalen < strlen("SIP/2.0 200"))
> >                 return NF_ACCEPT;
> > @@ -1382,8 +1383,13 @@ static int process_sip_response(struct sk_buff *skb, unsigned int protoff,
> >                 nf_ct_helper_log(skb, ct, "cannot parse cseq");
> >                 return NF_DROP;
> >         }
> > -       cseq = simple_strtoul(*dptr + matchoff, NULL, 10);
> > -       if (!cseq) {
>
> I think there is no need to convert simple_strtoul to kstrtouint, add
> a further check seems better?
> Like this:
>  -       if (!cseq) {
> +       if (!cseq && *(*dptr + matchoff) != '0') {

And what about an invalid CSeq that would look like CSeq: 0abzk852 ?
Should we check it is 0 + space instead ?

> > +       if (matchlen > sizeof(buf) - 1) {
> > +               nf_ct_helper_log(skb, ct, "cannot parse cseq (too big)");
> > +               return NF_DROP;
> > +       }
> > +       memcpy(buf, *dptr + matchoff, matchlen);
> > +       buf[matchlen] = 0;
> > +       if (kstrtouint(buf, 10, &cseq)) {
> >                 nf_ct_helper_log(skb, ct, "cannot get cseq");
> >                 return NF_DROP;
> >         }