netdev - Re: RCU'ed dst_get

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: <1322606542.2596.43.camel@edumazet-laptop>
Date:	Tue, 29 Nov 2011 23:42:22 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Marc Aurele La France <tsi@...berta.ca>
Cc:	Roland Dreier <roland@...nel.org>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	linux-rdma@...r.kernel.org
Subject: Re: RCU'ed dst_get_neighbour()

Le mardi 29 novembre 2011 à 15:32 -0700, Marc Aurele La France a écrit :
> On Tue, 29 Nov 2011, Eric Dumazet wrote:
> 
> > Le mardi 29 novembre 2011 à 22:17 +0100, Eric Dumazet a écrit :
> >> Le mardi 29 novembre 2011 à 14:00 -0700, Marc Aurele La France a écrit :
> >>> On Tue, 29 Nov 2011, Eric Dumazet wrote:
> >>>> Oh well, I forgot one rcu_read_unlock(), I'll send a V2...
> 
> >>> This also doesn't address the other dst_get_neighbour() instances
> >>> introduced by
> >>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=69cce1d1404968f78b177a0314f5822d5afdbbfb
> 
> >> Oh well, a complete audit is needed, and I have no choice but doing it.
> 
> > Here is the result of this audit, please double check and test it, I
> > only compiled this.
> 
> > [PATCH V2] drivers/infiniband: fix lockdep splats
> 
> > commit f2c31e32b37 (net: fix NULL dereferences in check_peer_redir())
> > forgot to take care of infiniband uses of dst neighbours.
> 
> > Many thanks to Marc Aurele who provided a nice bug report and feedback.
> 
> > Reported-by: Marc Aurele La France <tsi@...berta.ca>
> > Signed-off-by: Eric Dumazet <eric.dumazet@...il.com>
> > CC: David Miller <davem@...emloft.net>
> > CC: Roland Dreier <roland@...nel.org>
> > ---
> > drivers/infiniband/core/addr.c                 |    9 +++++--
> > drivers/infiniband/hw/cxgb3/iwch_cm.c          |    4 +++
> > drivers/infiniband/hw/cxgb4/cm.c               |    6 +++++
> > drivers/infiniband/hw/nes/nes_cm.c             |    6 +++--
> > drivers/infiniband/ulp/ipoib/ipoib_main.c      |   18 +++++++++------
> > drivers/infiniband/ulp/ipoib/ipoib_multicast.c |    6 +++--
> > 6 files changed, 35 insertions(+), 14 deletions(-)
> 
> This looks good to me, although I'm a little iffy on your use of
> dst_get_neighbour_raw(), but that could be just me.
> 

If you only test the neigh pointer being null (or not null), you dont
need to rcu_read_lock() before.

> But your audit is incomplete, a grep of 3.1.3 for dst_get_neighbour() and 
> dst_get_neighbour_raw() reveals occurrences in ...
> 

:=)

> include/net/dst.h
> net/atm/clip.c
> net/sched/sch_teql.c
> net/core/neighbour.c
> net/ipv4/ip_gre.c
> net/ipv4/ip_output.c
> net/ipv4/route.c
> net/xfrm/xfrm_policy.c
> net/bridge/br_netfilter.c
> net/decnet/dn_neigh.c
> net/decnet/dn_route.c
> net/ipv6/sit.c
> net/ipv6/addrconf.c
> net/ipv6/route.c
> net/ipv6/ndisc.c
> net/ipv6/ip6_output.c
> net/ipv6/ip6_fib.c
> 

So you did a grep but did not an analysis, did you ?

All these are already covered, or else bad things would already
happened. 

Thanks


--
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