[PATCH net] ip6: fix skb leak in ip6frag_expire_frag_queue()

[PATCH net] ip6: fix skb leak in ip6frag_expire_frag_queue()

[Eric Dumazet]

Since ip6frag_expire_frag_queue() now pulls the head skb
from frag queue, we should no longer use skb_get(), since
this leads to an skb leak.

Stefan Bader initially reported a problem in 4.4.stable [1] caused
by the skb_get(), so this patch should also fix this issue.

296583.091021] kernel BUG at /build/linux-6VmqmP/linux-4.4.0/net/core/skbuff.c:1207!
[296583.091734] Call Trace:
[296583.091749]  [<ffffffff81740e50>] __pskb_pull_tail+0x50/0x350
[296583.091764]  [<ffffffff8183939a>] _decode_session6+0x26a/0x400
[296583.091779]  [<ffffffff817ec719>] __xfrm_decode_session+0x39/0x50
[296583.091795]  [<ffffffff818239d0>] icmpv6_route_lookup+0xf0/0x1c0
[296583.091809]  [<ffffffff81824421>] icmp6_send+0x5e1/0x940
[296583.091823]  [<ffffffff81753238>] ? __netif_receive_skb+0x18/0x60
[296583.091838]  [<ffffffff817532b2>] ? netif_receive_skb_internal+0x32/0xa0
[296583.091858]  [<ffffffffc0199f74>] ? ixgbe_clean_rx_irq+0x594/0xac0 [ixgbe]
[296583.091876]  [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6]
[296583.091893]  [<ffffffff8183d431>] icmpv6_send+0x21/0x30
[296583.091906]  [<ffffffff8182b500>] ip6_expire_frag_queue+0xe0/0x120
[296583.091921]  [<ffffffffc04eb27f>] nf_ct_frag6_expire+0x1f/0x30 [nf_defrag_ipv6]
[296583.091938]  [<ffffffff810f3b57>] call_timer_fn+0x37/0x140
[296583.091951]  [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6]
[296583.091968]  [<ffffffff810f5464>] run_timer_softirq+0x234/0x330
[296583.091982]  [<ffffffff8108a339>] __do_softirq+0x109/0x2b0

Fixes: d4289fcc9b16 ("net: IP6 defrag: use rbtrees for IPv6 defrag")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Stfan Bader <stefan.bader@canonical.com>
Cc: Peter Oskolkov <posk@google.com>
Cc: Florian Westphal <fw@strlen.de>
---
 include/net/ipv6_frag.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/net/ipv6_frag.h b/include/net/ipv6_frag.h
index 28aa9b30aeceac9a86ee6754e4b5809be115e947..1f77fb4dc79df6bc4e41d6d2f4d49ace32082ca4 100644
--- a/include/net/ipv6_frag.h
+++ b/include/net/ipv6_frag.h
@@ -94,7 +94,6 @@ ip6frag_expire_frag_queue(struct net *net, struct frag_queue *fq)
 		goto out;
 
 	head->dev = dev;
-	skb_get(head);
 	spin_unlock(&fq->q.lock);
 
 	icmpv6_send(head, ICMPV6_TIME_EXCEED, ICMPV6_EXC_FRAGTIME, 0);
-- 
2.21.0.1020.gf2820cf01a-goog

Re: [PATCH net] ip6: fix skb leak in ip6frag_expire_frag_queue()

[Nicolas Dichtel]

Le 03/05/2019 à 13:47, Eric Dumazet a écrit :
> Since ip6frag_expire_frag_queue() now pulls the head skb
> from frag queue, we should no longer use skb_get(), since
> this leads to an skb leak.
> 
> Stefan Bader initially reported a problem in 4.4.stable [1] caused
> by the skb_get(), so this patch should also fix this issue.
> 
> 296583.091021] kernel BUG at /build/linux-6VmqmP/linux-4.4.0/net/core/skbuff.c:1207!
> [296583.091734] Call Trace:
> [296583.091749]  [<ffffffff81740e50>] __pskb_pull_tail+0x50/0x350
> [296583.091764]  [<ffffffff8183939a>] _decode_session6+0x26a/0x400
> [296583.091779]  [<ffffffff817ec719>] __xfrm_decode_session+0x39/0x50
> [296583.091795]  [<ffffffff818239d0>] icmpv6_route_lookup+0xf0/0x1c0
> [296583.091809]  [<ffffffff81824421>] icmp6_send+0x5e1/0x940
> [296583.091823]  [<ffffffff81753238>] ? __netif_receive_skb+0x18/0x60
> [296583.091838]  [<ffffffff817532b2>] ? netif_receive_skb_internal+0x32/0xa0
> [296583.091858]  [<ffffffffc0199f74>] ? ixgbe_clean_rx_irq+0x594/0xac0 [ixgbe]
> [296583.091876]  [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6]
> [296583.091893]  [<ffffffff8183d431>] icmpv6_send+0x21/0x30
> [296583.091906]  [<ffffffff8182b500>] ip6_expire_frag_queue+0xe0/0x120
> [296583.091921]  [<ffffffffc04eb27f>] nf_ct_frag6_expire+0x1f/0x30 [nf_defrag_ipv6]
> [296583.091938]  [<ffffffff810f3b57>] call_timer_fn+0x37/0x140
> [296583.091951]  [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6]
> [296583.091968]  [<ffffffff810f5464>] run_timer_softirq+0x234/0x330
> [296583.091982]  [<ffffffff8108a339>] __do_softirq+0x109/0x2b0
> 
> Fixes: d4289fcc9b16 ("net: IP6 defrag: use rbtrees for IPv6 defrag")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Stfan Bader <stefan.bader@canonical.com>
nit: the 'e' is missing in Stefan ;-)

Re: [PATCH net] ip6: fix skb leak in ip6frag_expire_frag_queue()

[Eric Dumazet]

On Fri, May 3, 2019 at 10:55 AM Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
>
> Le 03/05/2019 à 13:47, Eric Dumazet a écrit :
> > Since ip6frag_expire_frag_queue() now pulls the head skb
> > from frag queue, we should no longer use skb_get(), since
> > this leads to an skb leak.
> >
> > Stefan Bader initially reported a problem in 4.4.stable [1] caused
> > by the skb_get(), so this patch should also fix this issue.
> >
> > 296583.091021] kernel BUG at /build/linux-6VmqmP/linux-4.4.0/net/core/skbuff.c:1207!
> > [296583.091734] Call Trace:
> > [296583.091749]  [<ffffffff81740e50>] __pskb_pull_tail+0x50/0x350
> > [296583.091764]  [<ffffffff8183939a>] _decode_session6+0x26a/0x400
> > [296583.091779]  [<ffffffff817ec719>] __xfrm_decode_session+0x39/0x50
> > [296583.091795]  [<ffffffff818239d0>] icmpv6_route_lookup+0xf0/0x1c0
> > [296583.091809]  [<ffffffff81824421>] icmp6_send+0x5e1/0x940
> > [296583.091823]  [<ffffffff81753238>] ? __netif_receive_skb+0x18/0x60
> > [296583.091838]  [<ffffffff817532b2>] ? netif_receive_skb_internal+0x32/0xa0
> > [296583.091858]  [<ffffffffc0199f74>] ? ixgbe_clean_rx_irq+0x594/0xac0 [ixgbe]
> > [296583.091876]  [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6]
> > [296583.091893]  [<ffffffff8183d431>] icmpv6_send+0x21/0x30
> > [296583.091906]  [<ffffffff8182b500>] ip6_expire_frag_queue+0xe0/0x120
> > [296583.091921]  [<ffffffffc04eb27f>] nf_ct_frag6_expire+0x1f/0x30 [nf_defrag_ipv6]
> > [296583.091938]  [<ffffffff810f3b57>] call_timer_fn+0x37/0x140
> > [296583.091951]  [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6]
> > [296583.091968]  [<ffffffff810f5464>] run_timer_softirq+0x234/0x330
> > [296583.091982]  [<ffffffff8108a339>] __do_softirq+0x109/0x2b0
> >
> > Fixes: d4289fcc9b16 ("net: IP6 defrag: use rbtrees for IPv6 defrag")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Reported-by: Stfan Bader <stefan.bader@canonical.com>
> nit: the 'e' is missing in Stefan ;-)

Indeed, copy/paste error, thanks.

Re: [PATCH net] ip6: fix skb leak in ip6frag_expire_frag_queue()

[Peter Oskolkov]

On Fri, May 3, 2019 at 4:47 AM Eric Dumazet <edumazet@google.com> wrote:
>
> Since ip6frag_expire_frag_queue() now pulls the head skb
> from frag queue, we should no longer use skb_get(), since
> this leads to an skb leak.
>
> Stefan Bader initially reported a problem in 4.4.stable [1] caused
> by the skb_get(), so this patch should also fix this issue.
>
> 296583.091021] kernel BUG at /build/linux-6VmqmP/linux-4.4.0/net/core/skbuff.c:1207!
> [296583.091734] Call Trace:
> [296583.091749]  [<ffffffff81740e50>] __pskb_pull_tail+0x50/0x350
> [296583.091764]  [<ffffffff8183939a>] _decode_session6+0x26a/0x400
> [296583.091779]  [<ffffffff817ec719>] __xfrm_decode_session+0x39/0x50
> [296583.091795]  [<ffffffff818239d0>] icmpv6_route_lookup+0xf0/0x1c0
> [296583.091809]  [<ffffffff81824421>] icmp6_send+0x5e1/0x940
> [296583.091823]  [<ffffffff81753238>] ? __netif_receive_skb+0x18/0x60
> [296583.091838]  [<ffffffff817532b2>] ? netif_receive_skb_internal+0x32/0xa0
> [296583.091858]  [<ffffffffc0199f74>] ? ixgbe_clean_rx_irq+0x594/0xac0 [ixgbe]
> [296583.091876]  [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6]
> [296583.091893]  [<ffffffff8183d431>] icmpv6_send+0x21/0x30
> [296583.091906]  [<ffffffff8182b500>] ip6_expire_frag_queue+0xe0/0x120
> [296583.091921]  [<ffffffffc04eb27f>] nf_ct_frag6_expire+0x1f/0x30 [nf_defrag_ipv6]
> [296583.091938]  [<ffffffff810f3b57>] call_timer_fn+0x37/0x140
> [296583.091951]  [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6]
> [296583.091968]  [<ffffffff810f5464>] run_timer_softirq+0x234/0x330
> [296583.091982]  [<ffffffff8108a339>] __do_softirq+0x109/0x2b0
>
> Fixes: d4289fcc9b16 ("net: IP6 defrag: use rbtrees for IPv6 defrag")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Stfan Bader <stefan.bader@canonical.com>
> Cc: Peter Oskolkov <posk@google.com>
> Cc: Florian Westphal <fw@strlen.de>
> ---
>  include/net/ipv6_frag.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/include/net/ipv6_frag.h b/include/net/ipv6_frag.h
> index 28aa9b30aeceac9a86ee6754e4b5809be115e947..1f77fb4dc79df6bc4e41d6d2f4d49ace32082ca4 100644
> --- a/include/net/ipv6_frag.h
> +++ b/include/net/ipv6_frag.h
> @@ -94,7 +94,6 @@ ip6frag_expire_frag_queue(struct net *net, struct frag_queue *fq)
>                 goto out;
>
>         head->dev = dev;
> -       skb_get(head);

This skb_get was introduced by commit 05c0b86b9696802fd0ce5676a92a63f1b455bdf3
"ipv6: frags: rewrite ip6_expire_frag_queue()", and the rbtree patch
is not in 4.4, where the bug is reported at.
Shouldn't the "Fixes" tag also reference the original patch?


>         spin_unlock(&fq->q.lock);
>
>         icmpv6_send(head, ICMPV6_TIME_EXCEED, ICMPV6_EXC_FRAGTIME, 0);
> --
> 2.21.0.1020.gf2820cf01a-goog
>

Re: [PATCH net] ip6: fix skb leak in ip6frag_expire_frag_queue()

[Eric Dumazet]

On Fri, May 3, 2019 at 11:33 AM Peter Oskolkov <posk@google.com> wrote:
>
> This skb_get was introduced by commit 05c0b86b9696802fd0ce5676a92a63f1b455bdf3
> "ipv6: frags: rewrite ip6_expire_frag_queue()", and the rbtree patch
> is not in 4.4, where the bug is reported at.
> Shouldn't the "Fixes" tag also reference the original patch?

No, this bug really fixes a memory leak.

Fact that it also fixes the XFRM issue is secondary, since all your
patches are being backported in stable
trees anyway for other reasons.

There is no need to list all commits and give a complete context for a
bug fix like this one,
this would be quite noisy.

Re: [PATCH net] ip6: fix skb leak in ip6frag_expire_frag_queue()

[Peter Oskolkov]

On Fri, May 3, 2019 at 8:52 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, May 3, 2019 at 11:33 AM Peter Oskolkov <posk@google.com> wrote:
> >
> > This skb_get was introduced by commit 05c0b86b9696802fd0ce5676a92a63f1b455bdf3
> > "ipv6: frags: rewrite ip6_expire_frag_queue()", and the rbtree patch
> > is not in 4.4, where the bug is reported at.
> > Shouldn't the "Fixes" tag also reference the original patch?
>
> No, this bug really fixes a memory leak.
>
> Fact that it also fixes the XFRM issue is secondary, since all your
> patches are being backported in stable
> trees anyway for other reasons.

There are no plans to backport rbtree patches to 4.4 and earlier at
the moment, afaik.

>
> There is no need to list all commits and give a complete context for a
> bug fix like this one,
> this would be quite noisy.

Re: [PATCH net] ip6: fix skb leak in ip6frag_expire_frag_queue()

[Eric Dumazet]

On 5/3/19 11:58 AM, Peter Oskolkov wrote:
> On Fri, May 3, 2019 at 8:52 AM Eric Dumazet <edumazet@google.com> wrote:
>>
>> On Fri, May 3, 2019 at 11:33 AM Peter Oskolkov <posk@google.com> wrote:
>>>
>>> This skb_get was introduced by commit 05c0b86b9696802fd0ce5676a92a63f1b455bdf3
>>> "ipv6: frags: rewrite ip6_expire_frag_queue()", and the rbtree patch
>>> is not in 4.4, where the bug is reported at.
>>> Shouldn't the "Fixes" tag also reference the original patch?
>>
>> No, this bug really fixes a memory leak.
>>
>> Fact that it also fixes the XFRM issue is secondary, since all your
>> patches are being backported in stable
>> trees anyway for other reasons.
> 
> There are no plans to backport rbtree patches to 4.4 and earlier at
> the moment, afaik.
> 

No problem, I mentioned to Stefan what needs to be done.

(removing the head skb, removing the skb_get())