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: <03151db3-61fe-478d-b91b-549d18648738@redhat.com>
Date: Thu, 3 Apr 2025 09:30:43 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Antoine Tenart <atenart@...nel.org>, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org,
 Steffen Klassert <steffen.klassert@...unet.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net] net: decrease cached dst counters in dst_release

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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ