[PATCH net 0/2] ipv6: fix issues on accessing fib6_metrics

[PATCH net 0/2] ipv6: fix issues on accessing fib6_metrics

[Wei Wang]

From: Wei Wang <weiwan@google.com>

The latest fix on the memory leak of fib6_metrics still causes
use-after-free.
This patch series first revert the previous fix and propose a new fix
that is more inline with ipv4 logic and is tested to fix the
use-after-free issue reported.

Wei Wang (2):
  Revert "ipv6: fix double refcount of fib6_metrics"
  ipv6: fix memory leak on dst->_metrics

 net/ipv6/route.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.19.0.397.gdd90340f6a-goog

[PATCH net 1/2] Revert "ipv6: fix double refcount of fib6_metrics"

[Wei Wang]

From: Wei Wang <weiwan@google.com>

This reverts commit e70a3aad44cc8b24986687ffc98c4a4f6ecf25ea.

This change causes use-after-free on dst->_metrics.
The crash trace looks like this:
[   97.763269] BUG: KASAN: use-after-free in ip6_mtu+0x116/0x140
[   97.769038] Read of size 4 at addr ffff881781d2cf84 by task svw_NetThreadEv/8801

[   97.777954] CPU: 76 PID: 8801 Comm: svw_NetThreadEv Not tainted 4.15.0-smp-DEV #11
[   97.777956] Hardware name: Default string Default string/Indus_QC_02, BIOS 5.46.4 03/29/2018
[   97.777957] Call Trace:
[   97.777971]  [<ffffffff895709db>] dump_stack+0x4d/0x72
[   97.777985]  [<ffffffff881651df>] print_address_description+0x6f/0x260
[   97.777997]  [<ffffffff88165747>] kasan_report+0x257/0x370
[   97.778001]  [<ffffffff894488e6>] ? ip6_mtu+0x116/0x140
[   97.778004]  [<ffffffff881658b9>] __asan_report_load4_noabort+0x19/0x20
[   97.778008]  [<ffffffff894488e6>] ip6_mtu+0x116/0x140
[   97.778013]  [<ffffffff892bb91e>] tcp_current_mss+0x12e/0x280
[   97.778016]  [<ffffffff892bb7f0>] ? tcp_mtu_to_mss+0x2d0/0x2d0
[   97.778022]  [<ffffffff887b45b8>] ? depot_save_stack+0x138/0x4a0
[   97.778037]  [<ffffffff87c38985>] ? __mmdrop+0x145/0x1f0
[   97.778040]  [<ffffffff881643b1>] ? save_stack+0xb1/0xd0
[   97.778046]  [<ffffffff89264c82>] tcp_send_mss+0x22/0x220
[   97.778059]  [<ffffffff89273a49>] tcp_sendmsg_locked+0x4f9/0x39f0
[   97.778062]  [<ffffffff881642b4>] ? kasan_check_write+0x14/0x20
[   97.778066]  [<ffffffff89273550>] ? tcp_sendpage+0x60/0x60
[   97.778070]  [<ffffffff881cb359>] ? rw_copy_check_uvector+0x69/0x280
[   97.778075]  [<ffffffff8873c65f>] ? import_iovec+0x9f/0x430
[   97.778078]  [<ffffffff88164be7>] ? kasan_slab_free+0x87/0xc0
[   97.778082]  [<ffffffff8873c5c0>] ? memzero_page+0x140/0x140
[   97.778085]  [<ffffffff881642b4>] ? kasan_check_write+0x14/0x20
[   97.778088]  [<ffffffff89276f6c>] tcp_sendmsg+0x2c/0x50
[   97.778092]  [<ffffffff89276f6c>] ? tcp_sendmsg+0x2c/0x50
[   97.778098]  [<ffffffff89352d43>] inet_sendmsg+0x103/0x480
[   97.778102]  [<ffffffff89352c40>] ? inet_gso_segment+0x15b0/0x15b0
[   97.778105]  [<ffffffff890294da>] sock_sendmsg+0xba/0xf0
[   97.778108]  [<ffffffff8902ab6a>] ___sys_sendmsg+0x6ca/0x8e0
[   97.778113]  [<ffffffff87dccac1>] ? hrtimer_try_to_cancel+0x71/0x3b0
[   97.778116]  [<ffffffff8902a4a0>] ? copy_msghdr_from_user+0x3d0/0x3d0
[   97.778119]  [<ffffffff881646d1>] ? memset+0x31/0x40
[   97.778123]  [<ffffffff87a0cff5>] ? schedule_hrtimeout_range_clock+0x165/0x380
[   97.778127]  [<ffffffff87a0ce90>] ? hrtimer_nanosleep_restart+0x250/0x250
[   97.778130]  [<ffffffff87dcc700>] ? __hrtimer_init+0x180/0x180
[   97.778133]  [<ffffffff87dd1f82>] ? ktime_get_ts64+0x172/0x200
[   97.778137]  [<ffffffff8822b8ec>] ? __fget_light+0x8c/0x2f0
[   97.778141]  [<ffffffff8902d5c6>] __sys_sendmsg+0xe6/0x190
[   97.778144]  [<ffffffff8902d5c6>] ? __sys_sendmsg+0xe6/0x190
[   97.778147]  [<ffffffff8902d4e0>] ? SyS_shutdown+0x20/0x20
[   97.778152]  [<ffffffff87cd4370>] ? wake_up_q+0xe0/0xe0
[   97.778155]  [<ffffffff8902d670>] ? __sys_sendmsg+0x190/0x190
[   97.778158]  [<ffffffff8902d683>] SyS_sendmsg+0x13/0x20
[   97.778162]  [<ffffffff87a1600c>] do_syscall_64+0x2ac/0x430
[   97.778166]  [<ffffffff87c17515>] ? do_page_fault+0x35/0x3d0
[   97.778171]  [<ffffffff8960131f>] ? page_fault+0x2f/0x50
[   97.778174]  [<ffffffff89600071>] entry_SYSCALL_64_after_hwframe+0x3d/0xa2
[   97.778177] RIP: 0033:0x7f83fa36000d
[   97.778178] RSP: 002b:00007f83ef9229e0 EFLAGS: 00000293 ORIG_RAX: 000000000000002e
[   97.778180] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007f83fa36000d
[   97.778182] RDX: 0000000000004000 RSI: 00007f83ef922f00 RDI: 0000000000000036
[   97.778183] RBP: 00007f83ef923040 R08: 00007f83ef9231f8 R09: 00007f83ef923168
[   97.778184] R10: 0000000000000000 R11: 0000000000000293 R12: 00007f83f69c5b40
[   97.778185] R13: 000000000000001c R14: 0000000000000001 R15: 0000000000004000

[   97.779684] Allocated by task 5919:
[   97.783185]  save_stack+0x46/0xd0
[   97.783187]  kasan_kmalloc+0xad/0xe0
[   97.783189]  kmem_cache_alloc_trace+0xdf/0x580
[   97.783190]  ip6_convert_metrics.isra.79+0x7e/0x190
[   97.783192]  ip6_route_info_create+0x60a/0x2480
[   97.783193]  ip6_route_add+0x1d/0x80
[   97.783195]  inet6_rtm_newroute+0xdd/0xf0
[   97.783198]  rtnetlink_rcv_msg+0x641/0xb10
[   97.783200]  netlink_rcv_skb+0x27b/0x3e0
[   97.783202]  rtnetlink_rcv+0x15/0x20
[   97.783203]  netlink_unicast+0x4be/0x720
[   97.783204]  netlink_sendmsg+0x7bc/0xbf0
[   97.783205]  sock_sendmsg+0xba/0xf0
[   97.783207]  ___sys_sendmsg+0x6ca/0x8e0
[   97.783208]  __sys_sendmsg+0xe6/0x190
[   97.783209]  SyS_sendmsg+0x13/0x20
[   97.783211]  do_syscall_64+0x2ac/0x430
[   97.783213]  entry_SYSCALL_64_after_hwframe+0x3d/0xa2

[   97.784709] Freed by task 0:
[   97.785056] knetbase: Error: /proc/sys/net/core/txcs_enable does not exist
[   97.794497]  save_stack+0x46/0xd0
[   97.794499]  kasan_slab_free+0x71/0xc0
[   97.794500]  kfree+0x7c/0xf0
[   97.794501]  fib6_info_destroy_rcu+0x24f/0x310
[   97.794504]  rcu_process_callbacks+0x38b/0x1730
[   97.794506]  __do_softirq+0x1c8/0x5d0

Reported-by: John Sperbeck <jsperbeck@google.com>
Signed-off-by: Wei Wang <weiwan@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/route.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 480a79f47c52..b5d3e6b294ab 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -976,6 +976,10 @@ static void rt6_set_from(struct rt6_info *rt, struct fib6_info *from)
 	rt->rt6i_flags &= ~RTF_EXPIRES;
 	rcu_assign_pointer(rt->from, from);
 	dst_init_metrics(&rt->dst, from->fib6_metrics->metrics, true);
+	if (from->fib6_metrics != &dst_default_metrics) {
+		rt->dst._metrics |= DST_METRICS_REFCOUNTED;
+		refcount_inc(&from->fib6_metrics->refcnt);
+	}
 }
 
 /* Caller must already hold reference to @ort */
-- 
2.19.0.397.gdd90340f6a-goog

[PATCH net 2/2] ipv6: fix memory leak on dst->_metrics

[Wei Wang]

From: Wei Wang <weiwan@google.com>

When dst->_metrics and f6i->fib6_metrics share the same memory, both
take reference count on the dst_metrics structure. However, when dst is
destroyed, ip6_dst_destroy() only invokes dst_destroy_metrics_generic()
which does not take care of READONLY metrics and does not release refcnt.
This causes memory leak.
Similar to ipv4 logic, the fix is to properly release refcnt and free
the memory space pointed by dst->_metrics if refcnt becomes 0.

Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
Reported-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Wei Wang <weiwan@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/route.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index b5d3e6b294ab..826b14de7dbb 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -364,11 +364,14 @@ EXPORT_SYMBOL(ip6_dst_alloc);
 
 static void ip6_dst_destroy(struct dst_entry *dst)
 {
+	struct dst_metrics *p = (struct dst_metrics *)DST_METRICS_PTR(dst);
 	struct rt6_info *rt = (struct rt6_info *)dst;
 	struct fib6_info *from;
 	struct inet6_dev *idev;
 
-	dst_destroy_metrics_generic(dst);
+	if (p != &dst_default_metrics && refcount_dec_and_test(&p->refcnt))
+		kfree(p);
+
 	rt6_uncached_list_del(rt);
 
 	idev = rt->rt6i_idev;
-- 
2.19.0.397.gdd90340f6a-goog

Re: [PATCH net 1/2] Revert "ipv6: fix double refcount of fib6_metrics"

[David Ahern]

On 9/18/18 1:44 PM, Wei Wang wrote:
> From: Wei Wang <weiwan@google.com>
> 
> This reverts commit e70a3aad44cc8b24986687ffc98c4a4f6ecf25ea.
> 
> This change causes use-after-free on dst->_metrics.

....

> Reported-by: John Sperbeck <jsperbeck@google.com>
> Signed-off-by: Wei Wang <weiwan@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv6/route.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [PATCH net 2/2] ipv6: fix memory leak on dst->_metrics

[David Ahern]

On 9/18/18 1:45 PM, Wei Wang wrote:
> From: Wei Wang <weiwan@google.com>
> 
> When dst->_metrics and f6i->fib6_metrics share the same memory, both
> take reference count on the dst_metrics structure. However, when dst is
> destroyed, ip6_dst_destroy() only invokes dst_destroy_metrics_generic()
> which does not take care of READONLY metrics and does not release refcnt.
> This causes memory leak.
> Similar to ipv4 logic, the fix is to properly release refcnt and free
> the memory space pointed by dst->_metrics if refcnt becomes 0.
> 
> Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
> Reported-by: Sabrina Dubroca <sd@queasysnail.net>
> Signed-off-by: Wei Wang <weiwan@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv6/route.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index b5d3e6b294ab..826b14de7dbb 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -364,11 +364,14 @@ EXPORT_SYMBOL(ip6_dst_alloc);
>  
>  static void ip6_dst_destroy(struct dst_entry *dst)
>  {
> +	struct dst_metrics *p = (struct dst_metrics *)DST_METRICS_PTR(dst);
>  	struct rt6_info *rt = (struct rt6_info *)dst;
>  	struct fib6_info *from;
>  	struct inet6_dev *idev;
>  
> -	dst_destroy_metrics_generic(dst);
> +	if (p != &dst_default_metrics && refcount_dec_and_test(&p->refcnt))
> +		kfree(p);
> +
>  	rt6_uncached_list_del(rt);
>  
>  	idev = rt->rt6i_idev;
> 

Reviewed-by: David Ahern <dsahern@gmail.com>

With the revert in patch 1 we are back to my original code after
93531c67431 ("net/ipv6: separate handling of FIB entries from dst based
routes").

My intention with that series was to make IPv6 handling of metrics as
identical to IPv4 as possible (v6 does have differences for example due
to autoconf and changing metrics after installing a route). The change
in this patch is what I missed back in April.

Comparing IPv4 and IPv6 code for memory allocation and freeing for FIB
entries, transferring metrics to dst_entry and cleanup of dst_entry all
look nearly identical - to the point that net-next could have common
helpers to manage the refcnt'ing. I can submit those after this change
hits net-next.

Thanks for your time getting to the bottom of the leak.

Re: [PATCH net 2/2] ipv6: fix memory leak on dst->_metrics

[Wei Wang]

On Tue, Sep 18, 2018 at 4:25 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 9/18/18 1:45 PM, Wei Wang wrote:
> > From: Wei Wang <weiwan@google.com>
> >
> > When dst->_metrics and f6i->fib6_metrics share the same memory, both
> > take reference count on the dst_metrics structure. However, when dst is
> > destroyed, ip6_dst_destroy() only invokes dst_destroy_metrics_generic()
> > which does not take care of READONLY metrics and does not release refcnt.
> > This causes memory leak.
> > Similar to ipv4 logic, the fix is to properly release refcnt and free
> > the memory space pointed by dst->_metrics if refcnt becomes 0.
> >
> > Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
> > Reported-by: Sabrina Dubroca <sd@queasysnail.net>
> > Signed-off-by: Wei Wang <weiwan@google.com>
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> >  net/ipv6/route.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > index b5d3e6b294ab..826b14de7dbb 100644
> > --- a/net/ipv6/route.c
> > +++ b/net/ipv6/route.c
> > @@ -364,11 +364,14 @@ EXPORT_SYMBOL(ip6_dst_alloc);
> >
> >  static void ip6_dst_destroy(struct dst_entry *dst)
> >  {
> > +     struct dst_metrics *p = (struct dst_metrics *)DST_METRICS_PTR(dst);
> >       struct rt6_info *rt = (struct rt6_info *)dst;
> >       struct fib6_info *from;
> >       struct inet6_dev *idev;
> >
> > -     dst_destroy_metrics_generic(dst);
> > +     if (p != &dst_default_metrics && refcount_dec_and_test(&p->refcnt))
> > +             kfree(p);
> > +
> >       rt6_uncached_list_del(rt);
> >
> >       idev = rt->rt6i_idev;
> >
>
> Reviewed-by: David Ahern <dsahern@gmail.com>
>
> With the revert in patch 1 we are back to my original code after
> 93531c67431 ("net/ipv6: separate handling of FIB entries from dst based
> routes").
>
> My intention with that series was to make IPv6 handling of metrics as
> identical to IPv4 as possible (v6 does have differences for example due
> to autoconf and changing metrics after installing a route). The change
> in this patch is what I missed back in April.
>
> Comparing IPv4 and IPv6 code for memory allocation and freeing for FIB
> entries, transferring metrics to dst_entry and cleanup of dst_entry all
> look nearly identical - to the point that net-next could have common
> helpers to manage the refcnt'ing. I can submit those after this change
> hits net-next.
Yes. Agree. Since both v4 and v6 use the same logic on handling
metrics, helpers can be useful.

>
> Thanks for your time getting to the bottom of the leak.

Re: [PATCH net 0/2] ipv6: fix issues on accessing fib6_metrics

[David Miller]

From: Wei Wang <weiwan@google.com>
Date: Tue, 18 Sep 2018 13:44:58 -0700

> From: Wei Wang <weiwan@google.com>
> 
> The latest fix on the memory leak of fib6_metrics still causes
> use-after-free.
> This patch series first revert the previous fix and propose a new fix
> that is more inline with ipv4 logic and is tested to fix the
> use-after-free issue reported.

Series applied.