[PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again

[PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again

[Guillaume Nault]

When fixing the skb leak introduced by the conversion to rbtree, I
forgot about the special case of duplicate fragments. The condition
under the 'insert_error' label isn't effective anymore as
nf_ct_frg6_gather() doesn't override the returned value anymore. So
duplicate fragments now get NF_DROP verdict.

To accept duplicate fragments again, handle them specially as soon as
inet_frag_queue_insert() reports them. Return -EINPROGRESS which will
translate to NF_STOLEN verdict, like any accepted fragment. However,
such packets don't carry any new information and aren't queued, so we
just drop them immediately.

Fixes: a0d56cb911ca ("netfilter: ipv6: nf_defrag: fix leakage of unqueued fragments")
Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 net/ipv6/netfilter/nf_conntrack_reasm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 5b3f65e29b6f..8951de8b568f 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -265,8 +265,14 @@ static int nf_ct_frag6_queue(struct frag_queue *fq, struct sk_buff *skb,
 
 	prev = fq->q.fragments_tail;
 	err = inet_frag_queue_insert(&fq->q, skb, offset, end);
-	if (err)
+	if (err) {
+		if (err == IPFRAG_DUP) {
+			/* No error for duplicates, pretend they got queued. */
+			kfree_skb(skb);
+			return -EINPROGRESS;
+		}
 		goto insert_error;
+	}
 
 	if (dev)
 		fq->iif = dev->ifindex;
@@ -304,8 +310,6 @@ static int nf_ct_frag6_queue(struct frag_queue *fq, struct sk_buff *skb,
 	return -EINPROGRESS;
 
 insert_error:
-	if (err == IPFRAG_DUP)
-		goto err;
 	inet_frag_kill(&fq->q);
 err:
 	skb_dst_drop(skb);
-- 
2.20.1

Re: [PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again

[Florian Westphal]

Guillaume Nault <gnault@redhat.com> wrote:
> When fixing the skb leak introduced by the conversion to rbtree, I
> forgot about the special case of duplicate fragments. The condition
> under the 'insert_error' label isn't effective anymore as
> nf_ct_frg6_gather() doesn't override the returned value anymore. So
> duplicate fragments now get NF_DROP verdict.
> 
> To accept duplicate fragments again, handle them specially as soon as
> inet_frag_queue_insert() reports them. Return -EINPROGRESS which will
> translate to NF_STOLEN verdict, like any accepted fragment. However,
> such packets don't carry any new information and aren't queued, so we
> just drop them immediately.

Why is this patch needed?

Whats the difference between

NF_DROP and kfree_skb+NF_STOLEN?

AFAICS this patch isn't needed, as nothing is broken, what am I missing?

Re: [PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again

[Guillaume Nault]

On Thu, Jun 06, 2019 at 06:29:30PM +0200, Florian Westphal wrote:
> Guillaume Nault <gnault@redhat.com> wrote:
> > When fixing the skb leak introduced by the conversion to rbtree, I
> > forgot about the special case of duplicate fragments. The condition
> > under the 'insert_error' label isn't effective anymore as
> > nf_ct_frg6_gather() doesn't override the returned value anymore. So
> > duplicate fragments now get NF_DROP verdict.
> > 
> > To accept duplicate fragments again, handle them specially as soon as
> > inet_frag_queue_insert() reports them. Return -EINPROGRESS which will
> > translate to NF_STOLEN verdict, like any accepted fragment. However,
> > such packets don't carry any new information and aren't queued, so we
> > just drop them immediately.
> 
> Why is this patch needed?
> 
> Whats the difference between
> 
> NF_DROP and kfree_skb+NF_STOLEN?
> 
> AFAICS this patch isn't needed, as nothing is broken, what am I missing?
> 
If the fragment was generated locally, then NF_DROP propagates the EPERM
error back to the sender, which breaks the ip_defrag selftest.

Re: [PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again

[Florian Westphal]

Guillaume Nault <gnault@redhat.com> wrote:
> On Thu, Jun 06, 2019 at 06:29:30PM +0200, Florian Westphal wrote:
> > Guillaume Nault <gnault@redhat.com> wrote:
> > > When fixing the skb leak introduced by the conversion to rbtree, I
> > > forgot about the special case of duplicate fragments. The condition
> > > under the 'insert_error' label isn't effective anymore as
> > > nf_ct_frg6_gather() doesn't override the returned value anymore. So
> > > duplicate fragments now get NF_DROP verdict.
> > > 
> > > To accept duplicate fragments again, handle them specially as soon as
> > > inet_frag_queue_insert() reports them. Return -EINPROGRESS which will
> > > translate to NF_STOLEN verdict, like any accepted fragment. However,
> > > such packets don't carry any new information and aren't queued, so we
> > > just drop them immediately.
> > 
> > Why is this patch needed?
> > 
> > Whats the difference between
> > 
> > NF_DROP and kfree_skb+NF_STOLEN?
> > 
> > AFAICS this patch isn't needed, as nothing is broken, what am I missing?
> > 
> If the fragment was generated locally, then NF_DROP propagates the EPERM
> error back to the sender, which breaks the ip_defrag selftest.

Indeed, I forgot about local case and error propagation, thanks for
explaining.

Re: [PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again

[Pablo Neira Ayuso]

On Thu, Jun 06, 2019 at 06:04:00PM +0200, Guillaume Nault wrote:
> When fixing the skb leak introduced by the conversion to rbtree, I
> forgot about the special case of duplicate fragments. The condition
> under the 'insert_error' label isn't effective anymore as
> nf_ct_frg6_gather() doesn't override the returned value anymore. So
> duplicate fragments now get NF_DROP verdict.
> 
> To accept duplicate fragments again, handle them specially as soon as
> inet_frag_queue_insert() reports them. Return -EINPROGRESS which will
> translate to NF_STOLEN verdict, like any accepted fragment. However,
> such packets don't carry any new information and aren't queued, so we
> just drop them immediately.

Applied, thanks.