* [PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again
@ 2019-06-06 16:04 Guillaume Nault
2019-06-06 16:29 ` Florian Westphal
2019-06-07 12:49 ` Pablo Neira Ayuso
0 siblings, 2 replies; 5+ messages in thread
From: Guillaume Nault @ 2019-06-06 16:04 UTC (permalink / raw)
To: netfilter-devel; +Cc: Peter Oskolkov, Florian Westphal, Pablo Neira Ayuso
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again
2019-06-06 16:04 [PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again Guillaume Nault
@ 2019-06-06 16:29 ` Florian Westphal
2019-06-06 17:58 ` Guillaume Nault
2019-06-07 12:49 ` Pablo Neira Ayuso
1 sibling, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2019-06-06 16:29 UTC (permalink / raw)
To: Guillaume Nault
Cc: netfilter-devel, Peter Oskolkov, Florian Westphal, Pablo Neira Ayuso
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again
2019-06-06 16:29 ` Florian Westphal
@ 2019-06-06 17:58 ` Guillaume Nault
2019-06-06 19:54 ` Florian Westphal
0 siblings, 1 reply; 5+ messages in thread
From: Guillaume Nault @ 2019-06-06 17:58 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel, Peter Oskolkov, Pablo Neira Ayuso
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again
2019-06-06 17:58 ` Guillaume Nault
@ 2019-06-06 19:54 ` Florian Westphal
0 siblings, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2019-06-06 19:54 UTC (permalink / raw)
To: Guillaume Nault
Cc: Florian Westphal, netfilter-devel, Peter Oskolkov, Pablo Neira Ayuso
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again
2019-06-06 16:04 [PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again Guillaume Nault
2019-06-06 16:29 ` Florian Westphal
@ 2019-06-07 12:49 ` Pablo Neira Ayuso
1 sibling, 0 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2019-06-07 12:49 UTC (permalink / raw)
To: Guillaume Nault; +Cc: netfilter-devel, Peter Oskolkov, Florian Westphal
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-07 12:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-06 16:04 [PATCH nf] netfilter: ipv6: nf_defrag: accept duplicate fragments again Guillaume Nault
2019-06-06 16:29 ` Florian Westphal
2019-06-06 17:58 ` Guillaume Nault
2019-06-06 19:54 ` Florian Westphal
2019-06-07 12:49 ` Pablo Neira Ayuso
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.