netdev - Re: [PATCH 1/2] xfrm: Fix refcount imbalance in xfrm

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]

Date:	Wed, 23 Jul 2014 15:09:38 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	eric.dumazet@...il.com
Cc:	steffen.klassert@...unet.com, herbert@...dor.apana.org.au,
	netdev@...r.kernel.org
Subject: Re: [PATCH 1/2] xfrm: Fix refcount imbalance in xfrm_lookup

From: Eric Dumazet <eric.dumazet@...il.com>
Date: Wed, 23 Jul 2014 19:21:13 +0200

> On Wed, 2014-07-23 at 10:18 +0200, Steffen Klassert wrote:
>> xfrm_lookup must return a dst_entry with a refcount for the caller.
>> Git commit 1a1ccc96abb ("xfrm: Remove caching of xfrm_policy_sk_bundles")
>> removed this refcount for the socket policy case accidentally.
>> This patch restores it and sets DST_NOCACHE flag to make sure
>> that the dst_entry is freed when the refcount becomes null.
>> 
>> Fixes: 1a1ccc96abb ("xfrm: Remove caching of xfrm_policy_sk_bundles")
>> Signed-off-by: Steffen Klassert <steffen.klassert@...unet.com>
>> ---
>>  net/xfrm/xfrm_policy.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
>> index a8ef510..0525d78 100644
>> --- a/net/xfrm/xfrm_policy.c
>> +++ b/net/xfrm/xfrm_policy.c
>> @@ -2097,6 +2097,8 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
>>  				goto no_transform;
>>  			}
>>  
>> +			dst_hold(&xdst->u.dst);
>> +			xdst->u.dst.flags |= DST_NOCACHE;
>>  			route = xdst->route;
>>  		}
>>  	}
> 
> 
> Hmm... are you sure ?
> 
> Before 1a1ccc96abb we did a dst_hold() because we kept the dst in
> xfrm_policy_sk_bundles. So keeping a reference count was mandatory.
> 
> But after, I don't understand why it is needed at all.

It should be needed.

We create a new bundle here, from scratch, based upon the socket
policy.

Such new bundles make routes with dst refcount == 0.

All callers expect that the route given back to them is either
the original 'dst' passed into xfrm_lookup() unmolested, or a
new 'dst' having a refcount taken for this caller.

That's why all call sites expect that they can just go
"dst_release(dst)" and these objects it works.

So, if we returned 'dst' with refcount == 0 it would not work.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html