Re: Use after free in __dst_destroy_metrics_generic

[Cong Wang]

On Sat, Sep 16, 2017 at 5:40 AM, Julian Anastasov  wrote:
>
> Hello,
>
> On Fri, 15 Sep 2017, Subash Abhinov Kasiviswanathan wrote:
>
>> > May be I'm missing some posting but I don't see if
>> > the patch was tested successfully.
>> >
>> Hi Julian
>>
>> I've had this patch being tested for the last 3-4 days in our regression rack
>> and I haven't seen the same issue being reproduced or even a related crash
>> or leak in dst.
>
> For now I see only its bug-hiding side effects, i.e.
> it does not call kfree. Now if there is no double-free
> there should be a leak, not for dst but for metrics.


You make very good points. I need to take a deeper look.


>
>> The original issue was reported only once to us from the regression rack only
>> so the exact steps to reproduce is unknown.
>
> OK, lets see, may be others can explain what happens.
>

This makes me thinking if it is possible that we no longer have
the guarantee of metrics address aligned to at least 4?
This seems unlikely since kmalloc() should return at least
word-size-aligned address.

Re: Use after free in __dst_destroy_metrics_generic

[Cong Wang]

On Fri, Sep 15, 2017 at 2:00 PM, Eric Dumazet  wrote:
>
> Hi Cong
>
> I believe your patch makes a lot of sense, please submit it formally ?
>

I have been waiting for Subash's testing, since I myself never even run it.

Re: Use after free in __dst_destroy_metrics_generic

[Julian Anastasov]

Hello,

On Fri, 15 Sep 2017, Subash Abhinov Kasiviswanathan wrote:

> > May be I'm missing some posting but I don't see if
> > the patch was tested successfully.
> > 
> Hi Julian
> 
> I've had this patch being tested for the last 3-4 days in our regression rack
> and I haven't seen the same issue being reproduced or even a related crash
> or leak in dst.

For now I see only its bug-hiding side effects, i.e.
it does not call kfree. Now if there is no double-free
there should be a leak, not for dst but for metrics.

> The original issue was reported only once to us from the regression rack only
> so the exact steps to reproduce is unknown.

OK, lets see, may be others can explain what happens.

Regards

--
Julian Anastasov 

Re: Use after free in __dst_destroy_metrics_generic

[Subash Abhinov Kasiviswanathan]

May be I'm missing some posting but I don't see if
the patch was tested successfully.

Regards

--
Julian Anastasov 


Hi Julian

I've had this patch being tested for the last 3-4 days in our regression 
rack
and I haven't seen the same issue being reproduced or even a related 
crash

or leak in dst.
The original issue was reported only once to us from the regression rack 
only

so the exact steps to reproduce is unknown.

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: Use after free in __dst_destroy_metrics_generic

[Julian Anastasov]

Hello,

On Fri, 15 Sep 2017, Eric Dumazet wrote:

> On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote:
> > On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
> >  wrote:
> > > We are seeing a possible use after free in ip6_dst_destroy.
> > >
> > > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some 
> > > path
> > > and allocated
> > > to ion driver. ion driver has also freed it. Finally the memory is freed 
> > > by
> > > the
> > > fib gc and crashes since it is already deallocated.
> > 
> > Does the attach (compile-only) patch help anything?
> > 
> > From my _quick_ glance, it seems we miss the refcnt'ing
> > right in __dst_destroy_metrics_generic().
> > 
> > Thanks!
> 
> 
> Hi Cong
> 
> I believe your patch makes a lot of sense, please submit it formally ?

Cong's patch is wrong for few reasons:

- it will stop to kfree non-refcounted metrics

- report was for IPV6 and we set DST_METRICS_REFCOUNTED only
for IPv4, for DST_METRICS_READ_ONLY metrics

- __dst_destroy_metrics_generic is called for val without
DST_METRICS_READ_ONLY flag and such metrics are not with
DST_METRICS_REFCOUNTED flag

- ->cow_metrics and dst_cow_metrics_generic are called with 
DST_METRICS_READ_ONLY flag set, there is no chance to write
new value twice, especially to write DST_METRICS_REFCOUNTED flag
and later to see this flag in __dst_destroy_metrics_generic

So, I'm not sure where exactly is the bug with the
metrics.

May be I'm missing some posting but I don't see if
the patch was tested successfully.

Regards

--
Julian Anastasov 

Re: Use after free in __dst_destroy_metrics_generic

[Eric Dumazet]

On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote:
> On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
>  wrote:
> > We are seeing a possible use after free in ip6_dst_destroy.
> >
> > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path
> > and allocated
> > to ion driver. ion driver has also freed it. Finally the memory is freed by
> > the
> > fib gc and crashes since it is already deallocated.
> 
> Does the attach (compile-only) patch help anything?
> 
> From my _quick_ glance, it seems we miss the refcnt'ing
> right in __dst_destroy_metrics_generic().
> 
> Thanks!


Hi Cong

I believe your patch makes a lot of sense, please submit it formally ?

Thanks !

Re: Use after free in __dst_destroy_metrics_generic

[Subash Abhinov Kasiviswanathan]

On 2017-09-08 10:10, Cong Wang wrote:

On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
 wrote:

We are seeing a possible use after free in ip6_dst_destroy.

It appears as if memory of the __DST_METRICS_PTR(old) was freed in 
some path

and allocated
to ion driver. ion driver has also freed it. Finally the memory is 
freed by

the
fib gc and crashes since it is already deallocated.


Does the attach (compile-only) patch help anything?

From my _quick_ glance, it seems we miss the refcnt'ing
right in __dst_destroy_metrics_generic().

Thanks!


Hi Cong

Thanks for patch. I'll try this out.

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: Use after free in __dst_destroy_metrics_generic

[Eric Dumazet]

On Fri, 2017-09-08 at 10:19 -0700, David Miller wrote:
> From: Eric Dumazet 
> Date: Fri, 08 Sep 2017 10:16:53 -0700
> 
> > On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote:
> >> On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
> >>  wrote:
> >> > We are seeing a possible use after free in ip6_dst_destroy.
> >> >
> >> > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some 
> >> > path
> >> > and allocated
> >> > to ion driver. ion driver has also freed it. Finally the memory is freed 
> >> > by
> >> > the
> >> > fib gc and crashes since it is already deallocated.
> >> 
> >> Does the attach (compile-only) patch help anything?
> >> 
> >> From my _quick_ glance, it seems we miss the refcnt'ing
> >> right in __dst_destroy_metrics_generic().
> >> 
> >> Thanks!
> > 
> > But 4.9 kernels do not have yet the DST_METRICS_REFCOUNTED thing,
> > since this was added in 4.12
> 
> It was backported via -stable.

I hate working on lazy bug reports.

Re: Use after free in __dst_destroy_metrics_generic

[David Miller]

From: Eric Dumazet 
Date: Fri, 08 Sep 2017 10:16:53 -0700

> On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote:
>> On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
>>  wrote:
>> > We are seeing a possible use after free in ip6_dst_destroy.
>> >
>> > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some 
>> > path
>> > and allocated
>> > to ion driver. ion driver has also freed it. Finally the memory is freed by
>> > the
>> > fib gc and crashes since it is already deallocated.
>> 
>> Does the attach (compile-only) patch help anything?
>> 
>> From my _quick_ glance, it seems we miss the refcnt'ing
>> right in __dst_destroy_metrics_generic().
>> 
>> Thanks!
> 
> But 4.9 kernels do not have yet the DST_METRICS_REFCOUNTED thing,
> since this was added in 4.12

It was backported via -stable.

Re: Use after free in __dst_destroy_metrics_generic

[Eric Dumazet]

On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote:
> On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
>  wrote:
> > We are seeing a possible use after free in ip6_dst_destroy.
> >
> > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path
> > and allocated
> > to ion driver. ion driver has also freed it. Finally the memory is freed by
> > the
> > fib gc and crashes since it is already deallocated.
> 
> Does the attach (compile-only) patch help anything?
> 
> From my _quick_ glance, it seems we miss the refcnt'ing
> right in __dst_destroy_metrics_generic().
> 
> Thanks!

But 4.9 kernels do not have yet the DST_METRICS_REFCOUNTED thing,
since this was added in 4.12

Re: Use after free in __dst_destroy_metrics_generic

[Stefano Brivio]

On Fri, 8 Sep 2017 09:12:09 -0700
Cong Wang  wrote:

> On Thu, Sep 7, 2017 at 5:56 PM, Stefano Brivio  wrote:
> > On Thu, 07 Sep 2017 18:52:02 -0600
> > Subash Abhinov Kasiviswanathan  wrote:
> >  
> >> We are seeing a possible use after free in ip6_dst_destroy.
> >>
> >> It appears as if memory of the __DST_METRICS_PTR(old) was freed in some
> >> path and allocated
> >> to ion driver. ion driver has also freed it. Finally the memory is freed
> >> by the
> >> fib gc and crashes since it is already deallocated.
> >>
> >> Target is running an ARM64 Android based 4.9 kernel.
> >> Issue was seen once on a regression rack (sorry, no reproducer).
> >> Any pointers to debug this is highly appreciated.
> >>
> >> [ 3489.470581] [] object_err+0x4c/0x5c
> >> [ 3489.470586] [] free_debug_processing+0x2e0/0x398
> >> [ 3489.470589] [] __slab_free+0x300/0x3e0
> >> [ 3489.470593] [] kfree+0x28c/0x290
> >> [ 3489.470601] []
> >> __dst_destroy_metrics_generic+0x6c/0x78
> >> [ 3489.470609] [] ip6_dst_destroy+0xb0/0xb4  
> >
> > Should be fixed by:
> >
> > commit ad65a2f05695aced349e308193c6e2a6b1d87112
> > Author: Wei Wang 
> > Date:   Sat Jun 17 10:42:35 2017 -0700
> >
> > ipv6: call dst_hold_safe() properly  
> 
> Obviously it should not. One is dst metric, the other is dst.

And obviously you're right. Sorry for the confusion, I blatantly
misread the backtrace.

--
Stefano

Re: Use after free in __dst_destroy_metrics_generic

[Cong Wang]

On Thu, Sep 7, 2017 at 5:56 PM, Stefano Brivio  wrote:
> On Thu, 07 Sep 2017 18:52:02 -0600
> Subash Abhinov Kasiviswanathan  wrote:
>
>> We are seeing a possible use after free in ip6_dst_destroy.
>>
>> It appears as if memory of the __DST_METRICS_PTR(old) was freed in some
>> path and allocated
>> to ion driver. ion driver has also freed it. Finally the memory is freed
>> by the
>> fib gc and crashes since it is already deallocated.
>>
>> Target is running an ARM64 Android based 4.9 kernel.
>> Issue was seen once on a regression rack (sorry, no reproducer).
>> Any pointers to debug this is highly appreciated.
>>
>> [ 3489.470581] [] object_err+0x4c/0x5c
>> [ 3489.470586] [] free_debug_processing+0x2e0/0x398
>> [ 3489.470589] [] __slab_free+0x300/0x3e0
>> [ 3489.470593] [] kfree+0x28c/0x290
>> [ 3489.470601] []
>> __dst_destroy_metrics_generic+0x6c/0x78
>> [ 3489.470609] [] ip6_dst_destroy+0xb0/0xb4
>
> Should be fixed by:
>
> commit ad65a2f05695aced349e308193c6e2a6b1d87112
> Author: Wei Wang 
> Date:   Sat Jun 17 10:42:35 2017 -0700
>
> ipv6: call dst_hold_safe() properly


Obviously it should not. One is dst metric, the other is dst.

Re: Use after free in __dst_destroy_metrics_generic

[Cong Wang]

On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
 wrote:
> We are seeing a possible use after free in ip6_dst_destroy.
>
> It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path
> and allocated
> to ion driver. ion driver has also freed it. Finally the memory is freed by
> the
> fib gc and crashes since it is already deallocated.

Does the attach (compile-only) patch help anything?

>From my _quick_ glance, it seems we miss the refcnt'ing
right in __dst_destroy_metrics_generic().

Thanks!
diff --git a/net/core/dst.c b/net/core/dst.c
index 00aa972ad1a1..b293aeae3018 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -241,8 +241,14 @@ void __dst_destroy_metrics_generic(struct dst_entry *dst, 
unsigned long old)
 
new = ((unsigned long) _default_metrics) | DST_METRICS_READ_ONLY;
prev = cmpxchg(>_metrics, old, new);
-   if (prev == old)
-   kfree(__DST_METRICS_PTR(old));
+   if (prev == old) {
+   struct dst_metrics *old_p = (struct dst_metrics 
*)__DST_METRICS_PTR(old);
+
+   if (prev & DST_METRICS_REFCOUNTED) {
+   if (atomic_dec_and_test(_p->refcnt))
+   kfree(old_p);
+   }
+   }
 }
 EXPORT_SYMBOL(__dst_destroy_metrics_generic);
 

Re: Use after free in __dst_destroy_metrics_generic

[Subash Abhinov Kasiviswanathan]

[ 3489.194392]  __ion_alloc+0x180/0x988


I do not see the __ion_alloc function in my tree.


Hi David

This function seems to be defined in an Android specific change.

https://android.googlesource.com/kernel/msm/+/20a5411d0115b16826f3d327b6abb0192c8a2001

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: Use after free in __dst_destroy_metrics_generic

[David Miller]

From: Subash Abhinov Kasiviswanathan 
Date: Thu, 07 Sep 2017 18:52:02 -0600

> [ 3489.194392]  __ion_alloc+0x180/0x988

I do not see the __ion_alloc function in my tree.

Re: Use after free in __dst_destroy_metrics_generic

[Subash Abhinov Kasiviswanathan]

Should be fixed by:

commit ad65a2f05695aced349e308193c6e2a6b1d87112
Author: Wei Wang 
Date:   Sat Jun 17 10:42:35 2017 -0700

ipv6: call dst_hold_safe() properly



Thanks for the info Stefano.

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: Use after free in __dst_destroy_metrics_generic

[Stefano Brivio]

On Thu, 07 Sep 2017 18:52:02 -0600
Subash Abhinov Kasiviswanathan  wrote:

> We are seeing a possible use after free in ip6_dst_destroy.
> 
> It appears as if memory of the __DST_METRICS_PTR(old) was freed in some 
> path and allocated
> to ion driver. ion driver has also freed it. Finally the memory is freed 
> by the
> fib gc and crashes since it is already deallocated.
> 
> Target is running an ARM64 Android based 4.9 kernel.
> Issue was seen once on a regression rack (sorry, no reproducer).
> Any pointers to debug this is highly appreciated.
> 
> [ 3489.470581] [] object_err+0x4c/0x5c
> [ 3489.470586] [] free_debug_processing+0x2e0/0x398
> [ 3489.470589] [] __slab_free+0x300/0x3e0
> [ 3489.470593] [] kfree+0x28c/0x290
> [ 3489.470601] [] 
> __dst_destroy_metrics_generic+0x6c/0x78
> [ 3489.470609] [] ip6_dst_destroy+0xb0/0xb4

Should be fixed by:

commit ad65a2f05695aced349e308193c6e2a6b1d87112
Author: Wei Wang 
Date:   Sat Jun 17 10:42:35 2017 -0700

ipv6: call dst_hold_safe() properly


--
Stefano

Use after free in __dst_destroy_metrics_generic

[Subash Abhinov Kasiviswanathan]

We are seeing a possible use after free in ip6_dst_destroy.

It appears as if memory of the __DST_METRICS_PTR(old) was freed in some 
path and allocated
to ion driver. ion driver has also freed it. Finally the memory is freed 
by the

fib gc and crashes since it is already deallocated.

Target is running an ARM64 Android based 4.9 kernel.
Issue was seen once on a regression rack (sorry, no reproducer).
Any pointers to debug this is highly appreciated.

[ 3489.470581] [] object_err+0x4c/0x5c
[ 3489.470586] [] free_debug_processing+0x2e0/0x398
[ 3489.470589] [] __slab_free+0x300/0x3e0
[ 3489.470593] [] kfree+0x28c/0x290
[ 3489.470601] [] 
__dst_destroy_metrics_generic+0x6c/0x78

[ 3489.470609] [] ip6_dst_destroy+0xb0/0xb4
[ 3489.470612] [] dst_destroy+0x88/0x174
[ 3489.470616] [] icmp6_dst_gc+0x90/0xc0
[ 3489.470621] [] fib6_gc_timer_cb+0x40/0xc0
[ 3489.470630] [] call_timer_fn+0x58/0x1d0
[ 3489.470635] [] expire_timers+0x100/0x18c
[ 3489.470638] [] run_timer_softirq+0x98/0x270
[ 3489.470642] [] __do_softirq+0x150/0x438
[ 3489.470649] [] irq_exit+0xe0/0x138

[ 3489.127227] 
=
[ 3489.135489] BUG kmalloc-128 (Tainted: GW  O   ): Object 
already free
[ 3489.142591] 
-

[ 3489.142591]
[ 3489.152313] Disabling lock debugging due to kernel taint
[ 3489.157688] INFO: Allocated in alloc_largest_available+0x58/0x1f0 
age=17 cpu=4 pid=649

[ 3489.165667]  alloc_debug_processing+0x114/0x1a0
[ 3489.170233]  ___slab_alloc.constprop.72+0x654/0x690
[ 3489.175150]  __slab_alloc.isra.68.constprop.71+0x48/0x80
[ 3489.180505]  kmem_cache_alloc_trace+0x198/0x2c4
[ 3489.185073]  alloc_largest_available+0x58/0x1f0
[ 3489.189643]  ion_system_heap_allocate+0x1b0/0x6e8
[ 3489.194392]  __ion_alloc+0x180/0x988
[ 3489.198004]  ion_ioctl+0x26c/0x590
[ 3489.201437]  do_vfs_ioctl+0xcc/0x888
[ 3489.205037]  SyS_ioctl+0x90/0xa4
[ 3489.208298]  el0_svc_naked+0x24/0x28
[ 3489.211909] INFO: Freed in process_info+0x188/0x1bc age=21 cpu=4 
pid=649

[ 3489.218661]  free_debug_processing+0x180/0x398
[ 3489.223137]  __slab_free+0x300/0x3e0
[ 3489.226745]  kfree+0x28c/0x290
[ 3489.229827]  process_info+0x188/0x1bc
[ 3489.233526]  ion_system_heap_allocate+0x4b4/0x6e8
[ 3489.238266]  __ion_alloc+0x180/0x988
[ 3489.241875]  ion_ioctl+0x26c/0x590
[ 3489.245308]  do_vfs_ioctl+0xcc/0x888
[ 3489.248917]  SyS_ioctl+0x90/0xa4
[ 3489.252171]  el0_svc_naked+0x24/0x28

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project