[PATCH net] net: decrease cached dst counters in dst_release

[PATCH net] net: decrease cached dst counters in dst_release

[Antoine Tenart]

Upstream fix ac888d58869b ("net: do not delay dst_entries_add() in
dst_release()") moved decrementing the dst count from dst_destroy to
dst_release to avoid accessing already freed data in case of netns
dismantle. However in case CONFIG_DST_CACHE is enabled and OvS+tunnels
are used, this fix is incomplete as the same issue will be seen for
cached dsts:

  Unable to handle kernel paging request at virtual address ffff5aabf6b5c000
  Call trace:
   percpu_counter_add_batch+0x3c/0x160 (P)
   dst_release+0xec/0x108
   dst_cache_destroy+0x68/0xd8
   dst_destroy+0x13c/0x168
   dst_destroy_rcu+0x1c/0xb0
   rcu_do_batch+0x18c/0x7d0
   rcu_core+0x174/0x378
   rcu_core_si+0x18/0x30

Fix this by invalidating the cache, and thus decrementing cached dst
counters, in dst_release too.

Fixes: d71785ffc7e7 ("net: add dst_cache to ovs vxlan lwtunnel")
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 net/core/dst.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/core/dst.c b/net/core/dst.c
index 9552a90d4772..6d76b799ce64 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -165,6 +165,14 @@ static void dst_count_dec(struct dst_entry *dst)
 void dst_release(struct dst_entry *dst)
 {
 	if (dst && rcuref_put(&dst->__rcuref)) {
+#ifdef CONFIG_DST_CACHE
+		if (dst->flags & DST_METADATA) {
+			struct metadata_dst *md_dst = (struct metadata_dst *)dst;
+
+			if (md_dst->type == METADATA_IP_TUNNEL)
+				dst_cache_reset_now(&md_dst->u.tun_info.dst_cache);
+		}
+#endif
 		dst_count_dec(dst);
 		call_rcu_hurry(&dst->rcu_head, dst_destroy_rcu);
 	}
-- 
2.49.0

Re: [PATCH net] net: decrease cached dst counters in dst_release

[Paolo Abeni]

On 3/26/25 6:36 PM, Antoine Tenart wrote:
> Upstream fix ac888d58869b ("net: do not delay dst_entries_add() in
> dst_release()") moved decrementing the dst count from dst_destroy to
> dst_release to avoid accessing already freed data in case of netns
> dismantle. However in case CONFIG_DST_CACHE is enabled and OvS+tunnels
> are used, this fix is incomplete as the same issue will be seen for
> cached dsts:
> 
>   Unable to handle kernel paging request at virtual address ffff5aabf6b5c000
>   Call trace:
>    percpu_counter_add_batch+0x3c/0x160 (P)
>    dst_release+0xec/0x108
>    dst_cache_destroy+0x68/0xd8
>    dst_destroy+0x13c/0x168
>    dst_destroy_rcu+0x1c/0xb0
>    rcu_do_batch+0x18c/0x7d0
>    rcu_core+0x174/0x378
>    rcu_core_si+0x18/0x30
> 
> Fix this by invalidating the cache, and thus decrementing cached dst
> counters, in dst_release too.
> 
> Fixes: d71785ffc7e7 ("net: add dst_cache to ovs vxlan lwtunnel")
> Signed-off-by: Antoine Tenart <atenart@kernel.org>


> ---
>  net/core/dst.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/core/dst.c b/net/core/dst.c
> index 9552a90d4772..6d76b799ce64 100644
> --- a/net/core/dst.c
> +++ b/net/core/dst.c
> @@ -165,6 +165,14 @@ static void dst_count_dec(struct dst_entry *dst)
>  void dst_release(struct dst_entry *dst)
>  {
>  	if (dst && rcuref_put(&dst->__rcuref)) {
> +#ifdef CONFIG_DST_CACHE
> +		if (dst->flags & DST_METADATA) {
> +			struct metadata_dst *md_dst = (struct metadata_dst *)dst;
> +
> +			if (md_dst->type == METADATA_IP_TUNNEL)
> +				dst_cache_reset_now(&md_dst->u.tun_info.dst_cache);

I think the fix is correct, but I'm wondering if we have a similar issue
for the METADATA_XFRM meta-dst. Isn't:

	dst_release(md_dst->u.xfrm_info.dst_orig);

in metadata_dst_free() going to cause the same UaF? Why don't we need to
clean such dst here, too?

Thanks!

Paolo

Re: [PATCH net] net: decrease cached dst counters in dst_release

[Antoine Tenart]

Hi Paolo,

Quoting Paolo Abeni (2025-04-01 10:00:56)
> On 3/26/25 6:36 PM, Antoine Tenart wrote:
> > 
> > diff --git a/net/core/dst.c b/net/core/dst.c
> > index 9552a90d4772..6d76b799ce64 100644
> > --- a/net/core/dst.c
> > +++ b/net/core/dst.c
> > @@ -165,6 +165,14 @@ static void dst_count_dec(struct dst_entry *dst)
> >  void dst_release(struct dst_entry *dst)
> >  {
> >       if (dst && rcuref_put(&dst->__rcuref)) {
> > +#ifdef CONFIG_DST_CACHE
> > +             if (dst->flags & DST_METADATA) {
> > +                     struct metadata_dst *md_dst = (struct metadata_dst *)dst;
> > +
> > +                     if (md_dst->type == METADATA_IP_TUNNEL)
> > +                             dst_cache_reset_now(&md_dst->u.tun_info.dst_cache);
> 
> I think the fix is correct, but I'm wondering if we have a similar issue
> for the METADATA_XFRM meta-dst. Isn't:
> 
>         dst_release(md_dst->u.xfrm_info.dst_orig);
> 
> in metadata_dst_free() going to cause the same UaF? Why don't we need to
> clean such dst here, too?

I don't know much about XFRM but if the orig_dst doesn't have
DST_NOCOUNT (which I guess is the case) you're right. Also Eric noted in
ac888d58869b,

"""
    1) in CONFIG_XFRM case, dst_destroy() can call
       dst_release_immediate(child), this might also cause UAF
       if the child does not have DST_NOCOUNT set.
       IPSEC maintainers might take a look and see how to address this.
"""

but here I'm not sure if that is the case nor of the implications of
moving that release.

As the dst_orig one seems logical I can move it to dst_release too, but
it seems a deeper look by XFRM experts would be needed in any way.

Thanks!
Antoine

Re: [PATCH net] net: decrease cached dst counters in dst_release

[Paolo Abeni]

Adding Steffen

On 4/2/25 10:03 AM, Antoine Tenart wrote:
> Quoting Paolo Abeni (2025-04-01 10:00:56)
>> On 3/26/25 6:36 PM, Antoine Tenart wrote:
>>>
>>> diff --git a/net/core/dst.c b/net/core/dst.c
>>> index 9552a90d4772..6d76b799ce64 100644
>>> --- a/net/core/dst.c
>>> +++ b/net/core/dst.c
>>> @@ -165,6 +165,14 @@ static void dst_count_dec(struct dst_entry *dst)
>>>  void dst_release(struct dst_entry *dst)
>>>  {
>>>       if (dst && rcuref_put(&dst->__rcuref)) {
>>> +#ifdef CONFIG_DST_CACHE
>>> +             if (dst->flags & DST_METADATA) {
>>> +                     struct metadata_dst *md_dst = (struct metadata_dst *)dst;
>>> +
>>> +                     if (md_dst->type == METADATA_IP_TUNNEL)
>>> +                             dst_cache_reset_now(&md_dst->u.tun_info.dst_cache);
>>
>> I think the fix is correct, but I'm wondering if we have a similar issue
>> for the METADATA_XFRM meta-dst. Isn't:
>>
>>         dst_release(md_dst->u.xfrm_info.dst_orig);
>>
>> in metadata_dst_free() going to cause the same UaF? Why don't we need to
>> clean such dst here, too?
> 
> I don't know much about XFRM but if the orig_dst doesn't have
> DST_NOCOUNT (which I guess is the case) you're right. Also Eric noted in
> ac888d58869b,
> 
> """
>     1) in CONFIG_XFRM case, dst_destroy() can call
>        dst_release_immediate(child), this might also cause UAF
>        if the child does not have DST_NOCOUNT set.
>        IPSEC maintainers might take a look and see how to address this.
> """
> 
> but here I'm not sure if that is the case nor of the implications of
> moving that release.
> 
> As the dst_orig one seems logical I can move it to dst_release too, but
> it seems a deeper look by XFRM experts would be needed in any way.

I also feel like the XFRM side needs some deeper look and the most
straight forward fix could have negative side effects, so I'm fine with
this patch dealing with dst_cache only.

@Steffen: could you please have a look at the possible UaF mentioned above?

Thanks,

Paolo

Re: [PATCH net] net: decrease cached dst counters in dst_release

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Wed, 26 Mar 2025 18:36:32 +0100 you wrote:
> Upstream fix ac888d58869b ("net: do not delay dst_entries_add() in
> dst_release()") moved decrementing the dst count from dst_destroy to
> dst_release to avoid accessing already freed data in case of netns
> dismantle. However in case CONFIG_DST_CACHE is enabled and OvS+tunnels
> are used, this fix is incomplete as the same issue will be seen for
> cached dsts:
> 
> [...]

Here is the summary with links:
  - [net] net: decrease cached dst counters in dst_release
    https://git.kernel.org/netdev/net/c/3a0a3ff6593d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html