netdev - Re: [PATCH] fix NULL pointer + success return in route lookup path

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:	Sat, 20 Jun 2009 13:11:16 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Jarek Poplawski <jarkao2@...il.com>
Cc:	netdev@...r.kernel.org, mbizon@...ebox.fr, dada1@...mosbay.com,
	kuznet@....inr.ac.ru, davem@...emloft.net, pekkas@...core.fi,
	jmorris@...ei.org, yoshfuji@...ux-ipv6.org
Subject: Re: [PATCH] fix NULL pointer + success return in route lookup path

On Sat, Jun 20, 2009 at 06:39:25PM +0200, Jarek Poplawski wrote:
> Jarek Poplawski wrote, On 06/20/2009 02:37 PM:
> 
> > Neil Horman wrote, On 06/19/2009 07:18 PM:
> > 
> >> Don't drop route if we're not caching	
> 
> ...
> 
> >>  route.c |   14 ++++++++++++--
> >>  1 file changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> >> index cd76b3c..65b3a8b 100644
> >> --- a/net/ipv4/route.c
> >> +++ b/net/ipv4/route.c
> >> @@ -1085,8 +1085,16 @@ restart:
> >>  	now = jiffies;
> >>  
> >>  	if (!rt_caching(dev_net(rt->u.dst.dev))) {
> >> -		rt_drop(rt);
> 
> 
> One more question: if this rt is assigned to an skb, there is only
> skb_dst_drop() in kfree_skb(), but it seems we skip rt_free() part,
> or I miss something?
> 
no, you're right.  That actually seems to be more general problem, independent
of the one I just fixed.  As I understand it, skb_dst_drop is intended to
release the reference on a route cache entry that it grabbed previously, but
specifically avoids going throught the free path that rt_drop implements
(ostensibly so that the garbage collector will clean it up later from the route
cache).  With the recent work that lets us disable the garbage collector
entirely, and simply not cache, this path seems like it will leak routes if an
skb holds the last reference to a route cache entry.  What we likely need to do
is enhance rt_caching to return true in the event that the garbage collector is
off or if the cache has been attacked (rebuilt too many times).  Then we need to
export that function and check it in skb_dst_drop, calling call_rcu_bh to free
the dst entry if rt_caching is false, and the dst entries refcnt is zero (or
something like that).  I'll look into this further and address it in a separate
thread.  Thanks, and good catch!
Neil

> Jarek P.
> 
> >> -		return 0;
> >> +		/*
> >> +		 * If we're not caching, just tell the caller we
> >> +		 * were successful and don't touch the route.  The
> >> +		 * caller hold the sole reference to the cache entry, and
> >> +		 * it will be released when the caller is done with it.
> >> +		 * If we drop it here, the callers have no way to resolve routes
> >> +		 * when we're not caching.  Instead, just point *rp at rt, so
> >> +		 * the caller gets a single use out of the route
> >> +		 */
> >> +		goto report_and_exit;
> >>  	}
> >>  
> >>  	rthp = &rt_hash_table[hash].chain;
> >> @@ -1217,6 +1225,8 @@ restart:
> >>  	rcu_assign_pointer(rt_hash_table[hash].chain, rt);
> >>  
> >>  	spin_unlock_bh(rt_hash_lock_addr(hash));
> >> +
> >> +report_and_exit:
> >>  	if (rp)
> >>  		*rp = rt;
> >>  	else
> 
> 
> 
--
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