netdev - Re: [PATCH net] net: decrease cached dst counters in dst

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <174358103232.4506.6967775691343340999@kwain>
Date: Wed, 02 Apr 2025 10:03:52 +0200
From: Antoine Tenart <atenart@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net] net: decrease cached dst counters in dst_release

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