[PATCH nf] netfilter: seqadj: Fix the wrong ack adjust for the RST packet without ack

[PATCH nf] netfilter: seqadj: Fix the wrong ack adjust for the RST packet without ack

[fgao]

From: Gao Feng <fgao@ikuai8.com>

It is valid that the TCP RST packet which does not set ack flag, and bytes
of ack number are zero. For these RST packets, seqadj could not adjust the
ack number.

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 v2: Regenerate because the first patch is removed
 v1: Initial patch

 net/netfilter/nf_conntrack_seqadj.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/net/netfilter/nf_conntrack_seqadj.c b/net/netfilter/nf_conntrack_seqadj.c
index dff0f0c..3bd9c7e 100644
--- a/net/netfilter/nf_conntrack_seqadj.c
+++ b/net/netfilter/nf_conntrack_seqadj.c
@@ -179,30 +179,34 @@ int nf_ct_seq_adjust(struct sk_buff *skb,
 
 	tcph = (void *)skb->data + protoff;
 	spin_lock_bh(&ct->lock);
+
 	if (after(ntohl(tcph->seq), this_way->correction_pos))
 		seqoff = this_way->offset_after;
 	else
 		seqoff = this_way->offset_before;
 
-	if (after(ntohl(tcph->ack_seq) - other_way->offset_before,
-		  other_way->correction_pos))
-		ackoff = other_way->offset_after;
-	else
-		ackoff = other_way->offset_before;
-
 	newseq = htonl(ntohl(tcph->seq) + seqoff);
-	newack = htonl(ntohl(tcph->ack_seq) - ackoff);
-
 	inet_proto_csum_replace4(&tcph->check, skb, tcph->seq, newseq, false);
-	inet_proto_csum_replace4(&tcph->check, skb, tcph->ack_seq, newack,
-				 false);
-
-	pr_debug("Adjusting sequence number from %u->%u, ack from %u->%u\n",
-		 ntohl(tcph->seq), ntohl(newseq), ntohl(tcph->ack_seq),
-		 ntohl(newack));
 
+	pr_debug("Adjusting sequence number from %u->%u\n",
+		 ntohl(tcph->seq), ntohl(newseq));
 	tcph->seq = newseq;
-	tcph->ack_seq = newack;
+
+	if (likely(tcph->ack)) {
+		if (after(ntohl(tcph->ack_seq) - other_way->offset_before,
+			  other_way->correction_pos))
+			ackoff = other_way->offset_after;
+		else
+			ackoff = other_way->offset_before;
+
+		newack = htonl(ntohl(tcph->ack_seq) - ackoff);
+		inet_proto_csum_replace4(&tcph->check, skb, tcph->ack_seq,
+					 newack, false);
+
+		pr_debug("Adjusting ack number from %u->%u\n",
+			 ntohl(tcph->ack_seq), ntohl(newack));
+		tcph->ack_seq = newack;
+	}
 
 	res = nf_ct_sack_adjust(skb, protoff, tcph, ct, ctinfo);
 	spin_unlock_bh(&ct->lock);
-- 
1.9.1

Re: [PATCH nf] netfilter: seqadj: Fix the wrong ack adjust for the RST packet without ack

[Feng Gao]

On Tue, Sep 6, 2016 at 10:06 AM,  <fgao@ikuai8.com> wrote:
> From: Gao Feng <fgao@ikuai8.com>
>
> It is valid that the TCP RST packet which does not set ack flag, and bytes
> of ack number are zero. For these RST packets, seqadj could not adjust the
> ack number.
>
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
>  v2: Regenerate because the first patch is removed
>  v1: Initial patch
>
>  net/netfilter/nf_conntrack_seqadj.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_seqadj.c b/net/netfilter/nf_conntrack_seqadj.c
> index dff0f0c..3bd9c7e 100644
> --- a/net/netfilter/nf_conntrack_seqadj.c
> +++ b/net/netfilter/nf_conntrack_seqadj.c
> @@ -179,30 +179,34 @@ int nf_ct_seq_adjust(struct sk_buff *skb,
>
>         tcph = (void *)skb->data + protoff;
>         spin_lock_bh(&ct->lock);
> +
>         if (after(ntohl(tcph->seq), this_way->correction_pos))
>                 seqoff = this_way->offset_after;
>         else
>                 seqoff = this_way->offset_before;
>
> -       if (after(ntohl(tcph->ack_seq) - other_way->offset_before,
> -                 other_way->correction_pos))
> -               ackoff = other_way->offset_after;
> -       else
> -               ackoff = other_way->offset_before;
> -
>         newseq = htonl(ntohl(tcph->seq) + seqoff);
> -       newack = htonl(ntohl(tcph->ack_seq) - ackoff);
> -
>         inet_proto_csum_replace4(&tcph->check, skb, tcph->seq, newseq, false);
> -       inet_proto_csum_replace4(&tcph->check, skb, tcph->ack_seq, newack,
> -                                false);
> -
> -       pr_debug("Adjusting sequence number from %u->%u, ack from %u->%u\n",
> -                ntohl(tcph->seq), ntohl(newseq), ntohl(tcph->ack_seq),
> -                ntohl(newack));
>
> +       pr_debug("Adjusting sequence number from %u->%u\n",
> +                ntohl(tcph->seq), ntohl(newseq));
>         tcph->seq = newseq;
> -       tcph->ack_seq = newack;
> +
> +       if (likely(tcph->ack)) {
> +               if (after(ntohl(tcph->ack_seq) - other_way->offset_before,
> +                         other_way->correction_pos))
> +                       ackoff = other_way->offset_after;
> +               else
> +                       ackoff = other_way->offset_before;
> +
> +               newack = htonl(ntohl(tcph->ack_seq) - ackoff);
> +               inet_proto_csum_replace4(&tcph->check, skb, tcph->ack_seq,
> +                                        newack, false);
> +
> +               pr_debug("Adjusting ack number from %u->%u\n",
> +                        ntohl(tcph->ack_seq), ntohl(newack));
> +               tcph->ack_seq = newack;
> +       }
>
>         res = nf_ct_sack_adjust(skb, protoff, tcph, ct, ctinfo);
>         spin_unlock_bh(&ct->lock);
> --
> 1.9.1
>
>

Sorry, I forget to add the v2 in the subject.

Best Regards
Feng

Re: [PATCH nf] netfilter: seqadj: Fix the wrong ack adjust for the RST packet without ack

[Pablo Neira Ayuso]

On Fri, Sep 09, 2016 at 11:06:51PM +0800, Feng Gao wrote:
> How about this patch ?

Is this a theoretical problem? I would really prefer if you do your
work to collect traces that show that the actual sequence adjustment
is not correct.

> I think it should be one bug of seqadjust.

"I think this is a bug" is not sufficient, I expect you prove this is
not correct.

Re: [PATCH nf] netfilter: seqadj: Fix the wrong ack adjust for the RST packet without ack

[Feng Gao]

Hi Pablo,

On Wed, Sep 14, 2016 at 5:51 PM, Feng Gao <gfree.wind@gmail.com> wrote:
>
>
> On Tue, Sep 13, 2016 at 1:17 AM, Pablo Neira Ayuso <pablo@netfilter.org>
> wrote:
>>
>> On Fri, Sep 09, 2016 at 11:06:51PM +0800, Feng Gao wrote:
>> > How about this patch ?
>>
>> Is this a theoretical problem? I would really prefer if you do your
>> work to collect traces that show that the actual sequence adjustment
>> is not correct.
>>
>> > I think it should be one bug of seqadjust.
>>
>> "I think this is a bug" is not sufficient, I expect you prove this is
>> not correct.
>
>
> Yes. Now I only analyze it by codes, and it should check ack flag in theory.
> Because TCP REST could not contain ack.
>
> But I will set up one environment and generate the packet trace.
>
> Regards
> Feng
>

I have gotten the packet trace which shows seqadj adjusts the 0 ack
value, and sent the v3 patch which contains the reproduce steps,
packet trace, and logs.

Best Regards
Feng

Re: [PATCH nf] netfilter: seqadj: Fix the wrong ack adjust for the RST packet without ack

[Pablo Neira Ayuso]

On Thu, Sep 22, 2016 at 10:36:22AM +0800, Feng Gao wrote:
> Hi Pablo,
> 
> On Wed, Sep 14, 2016 at 5:51 PM, Feng Gao <gfree.wind@gmail.com> wrote:
> >
> >
> > On Tue, Sep 13, 2016 at 1:17 AM, Pablo Neira Ayuso <pablo@netfilter.org>
> > wrote:
> >>
> >> On Fri, Sep 09, 2016 at 11:06:51PM +0800, Feng Gao wrote:
> >> > How about this patch ?
> >>
> >> Is this a theoretical problem? I would really prefer if you do your
> >> work to collect traces that show that the actual sequence adjustment
> >> is not correct.
> >>
> >> > I think it should be one bug of seqadjust.
> >>
> >> "I think this is a bug" is not sufficient, I expect you prove this is
> >> not correct.
> >
> >
> > Yes. Now I only analyze it by codes, and it should check ack flag in theory.
> > Because TCP REST could not contain ack.
> >
> > But I will set up one environment and generate the packet trace.
> >
> > Regards
> > Feng
> >
> 
> I have gotten the packet trace which shows seqadj adjusts the 0 ack
> value, and sent the v3 patch which contains the reproduce steps,
> packet trace, and logs.

Nice, thanks a lot.