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
| ||
|
Date: Mon, 01 Aug 2011 10:57:51 +0200 From: synapse <synapse@...py.csoma.elte.hu> To: Eric Dumazet <eric.dumazet@...il.com> CC: David Miller <davem@...emloft.net>, netdev@...r.kernel.org Subject: Re: PROBLEM: BUG (NULL ptr dereference in ipv4_dst_check) Hello Sorry, I wasn't home on the weekend. Exactly to which tree should I apply this? It doesn't apply cleanly to 3.0.0. Am I missing something? Gergely Kalman On 07/30/11 07:00, Eric Dumazet wrote: > Le vendredi 29 juillet 2011 à 17:43 +0200, Eric Dumazet a écrit : > >> Oh well, we already use RCU in neigh_destroy(), so adding rcu would need >> to change all dst_get_neighbour() callers to be in one rcu_read_lock() >> section. >> >> I'll take a look, I suspect its mostly already done. >> >> > Here is the patch I finally cooked and tested. > > Could you please test it as well Gergely ? > > Thanks ! > > [PATCH] net: fix NULL dereferences in check_peer_redir() > > Gergely Kalman reported crashes in check_peer_redir(). > > It appears commit f39925dbde778 (ipv4: Cache learned redirect > information in inetpeer.) added a race, leading to possible NULL ptr > dereference. > > Since we can now change dst neighbour, we should make sure a reader can > safely use a neighbour. > > Add RCU protection to dst neighbour, and make sure check_peer_redir() > can be called safely by different cpus in parallel. > > As neighbours are already freed after one RCU grace period, this patch > should not add typical RCU penalty (cache cold effects) > > Many thanks to Gergely for providing a pretty report pointing to the > bug. > > Reported-by: Gergely Kalman<synapse@...py.csoma.elte.hu> > Signed-off-by: Eric Dumazet<eric.dumazet@...il.com> > --- > include/net/dst.h | 17 +++++++++++++---- > net/ipv4/ip_output.c | 10 ++++++++-- > net/ipv4/route.c | 14 ++++++++------ > net/ipv6/addrconf.c | 2 +- > net/ipv6/ip6_fib.c | 2 +- > net/ipv6/ip6_output.c | 13 +++++++++++-- > net/ipv6/route.c | 35 +++++++++++++++++++++++++---------- > 7 files changed, 67 insertions(+), 26 deletions(-) > > diff --git a/include/net/dst.h b/include/net/dst.h > index 29e2557..13d507d 100644 > --- a/include/net/dst.h > +++ b/include/net/dst.h > @@ -37,7 +37,7 @@ struct dst_entry { > unsigned long _metrics; > unsigned long expires; > struct dst_entry *path; > - struct neighbour *_neighbour; > + struct neighbour __rcu *_neighbour; > #ifdef CONFIG_XFRM > struct xfrm_state *xfrm; > #else > @@ -88,12 +88,17 @@ struct dst_entry { > > static inline struct neighbour *dst_get_neighbour(struct dst_entry *dst) > { > - return dst->_neighbour; > + return rcu_dereference(dst->_neighbour); > +} > + > +static inline struct neighbour *dst_get_neighbour_raw(struct dst_entry *dst) > +{ > + return rcu_dereference_raw(dst->_neighbour); > } > > static inline void dst_set_neighbour(struct dst_entry *dst, struct neighbour *neigh) > { > - dst->_neighbour = neigh; > + rcu_assign_pointer(dst->_neighbour, neigh); > } > > extern u32 *dst_cow_metrics_generic(struct dst_entry *dst, unsigned long old); > @@ -382,8 +387,12 @@ static inline void dst_rcu_free(struct rcu_head *head) > static inline void dst_confirm(struct dst_entry *dst) > { > if (dst) { > - struct neighbour *n = dst_get_neighbour(dst); > + struct neighbour *n; > + > + rcu_read_lock(); > + n = dst_get_neighbour(dst); > neigh_confirm(n); > + rcu_read_unlock(); > } > } > > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c > index ccaaa85..77d3ede 100644 > --- a/net/ipv4/ip_output.c > +++ b/net/ipv4/ip_output.c > @@ -204,9 +204,15 @@ static inline int ip_finish_output2(struct sk_buff *skb) > skb = skb2; > } > > + rcu_read_lock(); > neigh = dst_get_neighbour(dst); > - if (neigh) > - return neigh_output(neigh, skb); > + if (neigh) { > + int res = neigh_output(neigh, skb); > + > + rcu_read_unlock(); > + return res; > + } > + rcu_read_unlock(); > > if (net_ratelimit()) > printk(KERN_DEBUG "ip_finish_output2: No header cache and no neighbour!\n"); > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > index 1730689..6afc4eb 100644 > --- a/net/ipv4/route.c > +++ b/net/ipv4/route.c > @@ -1628,16 +1628,18 @@ static int check_peer_redir(struct dst_entry *dst, struct inet_peer *peer) > { > struct rtable *rt = (struct rtable *) dst; > __be32 orig_gw = rt->rt_gateway; > - struct neighbour *n; > + struct neighbour *n, *old_n; > > dst_confirm(&rt->dst); > > - neigh_release(dst_get_neighbour(&rt->dst)); > - dst_set_neighbour(&rt->dst, NULL); > - > rt->rt_gateway = peer->redirect_learned.a4; > - rt_bind_neighbour(rt); > - n = dst_get_neighbour(&rt->dst); > + > + n = ipv4_neigh_lookup(&rt->dst,&rt->rt_gateway); > + if (IS_ERR(n)) > + return PTR_ERR(n); > + old_n = xchg(&rt->dst._neighbour, n); > + if (old_n) > + neigh_release(old_n); > if (!n || !(n->nud_state& NUD_VALID)) { > if (n) > neigh_event_send(n, NULL); > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index a55500c..f012ebd 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -656,7 +656,7 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, int pfxlen, > * layer address of our nexhop router > */ > > - if (dst_get_neighbour(&rt->dst) == NULL) > + if (dst_get_neighbour_raw(&rt->dst) == NULL) > ifa->flags&= ~IFA_F_OPTIMISTIC; > > ifa->idev = idev; > diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c > index 54a4678..320d91d 100644 > --- a/net/ipv6/ip6_fib.c > +++ b/net/ipv6/ip6_fib.c > @@ -1455,7 +1455,7 @@ static int fib6_age(struct rt6_info *rt, void *arg) > RT6_TRACE("aging clone %p\n", rt); > return -1; > } else if ((rt->rt6i_flags& RTF_GATEWAY)&& > - (!(dst_get_neighbour(&rt->dst)->flags& NTF_ROUTER))) { > + (!(dst_get_neighbour_raw(&rt->dst)->flags& NTF_ROUTER))) { > RT6_TRACE("purging route %p via non-router but gateway\n", > rt); > return -1; > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c > index 32e5339..4c882cf 100644 > --- a/net/ipv6/ip6_output.c > +++ b/net/ipv6/ip6_output.c > @@ -135,10 +135,15 @@ static int ip6_finish_output2(struct sk_buff *skb) > skb->len); > } > > + rcu_read_lock(); > neigh = dst_get_neighbour(dst); > - if (neigh) > - return neigh_output(neigh, skb); > + if (neigh) { > + int res = neigh_output(neigh, skb); > > + rcu_read_unlock(); > + return res; > + } > + rcu_read_unlock(); > IP6_INC_STATS_BH(dev_net(dst->dev), > ip6_dst_idev(dst), IPSTATS_MIB_OUTNOROUTES); > kfree_skb(skb); > @@ -975,12 +980,14 @@ static int ip6_dst_lookup_tail(struct sock *sk, > * dst entry and replace it instead with the > * dst entry of the nexthop router > */ > + rcu_read_lock(); > n = dst_get_neighbour(*dst); > if (n&& !(n->nud_state& NUD_VALID)) { > struct inet6_ifaddr *ifp; > struct flowi6 fl_gw6; > int redirect; > > + rcu_read_unlock(); > ifp = ipv6_get_ifaddr(net,&fl6->saddr, > (*dst)->dev, 1); > > @@ -1000,6 +1007,8 @@ static int ip6_dst_lookup_tail(struct sock *sk, > if ((err = (*dst)->error)) > goto out_err_release; > } > + } else { > + rcu_read_unlock(); > } > #endif > > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > index e8987da..9e69eb0 100644 > --- a/net/ipv6/route.c > +++ b/net/ipv6/route.c > @@ -364,7 +364,7 @@ out: > #ifdef CONFIG_IPV6_ROUTER_PREF > static void rt6_probe(struct rt6_info *rt) > { > - struct neighbour *neigh = rt ? dst_get_neighbour(&rt->dst) : NULL; > + struct neighbour *neigh; > /* > * Okay, this does not seem to be appropriate > * for now, however, we need to check if it > @@ -373,8 +373,10 @@ static void rt6_probe(struct rt6_info *rt) > * Router Reachability Probe MUST be rate-limited > * to no more than one per minute. > */ > + rcu_read_lock(); > + neigh = rt ? dst_get_neighbour(&rt->dst) : NULL; > if (!neigh || (neigh->nud_state& NUD_VALID)) > - return; > + goto out; > read_lock_bh(&neigh->lock); > if (!(neigh->nud_state& NUD_VALID)&& > time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) { > @@ -387,8 +389,11 @@ static void rt6_probe(struct rt6_info *rt) > target = (struct in6_addr *)&neigh->primary_key; > addrconf_addr_solict_mult(target,&mcaddr); > ndisc_send_ns(rt->rt6i_dev, NULL, target,&mcaddr, NULL); > - } else > + } else { > read_unlock_bh(&neigh->lock); > + } > +out: > + rcu_read_unlock(); > } > #else > static inline void rt6_probe(struct rt6_info *rt) > @@ -412,8 +417,11 @@ static inline int rt6_check_dev(struct rt6_info *rt, int oif) > > static inline int rt6_check_neigh(struct rt6_info *rt) > { > - struct neighbour *neigh = dst_get_neighbour(&rt->dst); > + struct neighbour *neigh; > int m; > + > + rcu_read_lock(); > + neigh = dst_get_neighbour(&rt->dst); > if (rt->rt6i_flags& RTF_NONEXTHOP || > !(rt->rt6i_flags& RTF_GATEWAY)) > m = 1; > @@ -430,6 +438,7 @@ static inline int rt6_check_neigh(struct rt6_info *rt) > read_unlock_bh(&neigh->lock); > } else > m = 0; > + rcu_read_unlock(); > return m; > } > > @@ -769,7 +778,7 @@ static struct rt6_info *rt6_alloc_clone(struct rt6_info *ort, > rt->rt6i_dst.plen = 128; > rt->rt6i_flags |= RTF_CACHE; > rt->dst.flags |= DST_HOST; > - dst_set_neighbour(&rt->dst, neigh_clone(dst_get_neighbour(&ort->dst))); > + dst_set_neighbour(&rt->dst, neigh_clone(dst_get_neighbour_raw(&ort->dst))); > } > return rt; > } > @@ -803,7 +812,7 @@ restart: > dst_hold(&rt->dst); > read_unlock_bh(&table->tb6_lock); > > - if (!dst_get_neighbour(&rt->dst)&& !(rt->rt6i_flags& RTF_NONEXTHOP)) > + if (!dst_get_neighbour_raw(&rt->dst)&& !(rt->rt6i_flags& RTF_NONEXTHOP)) > nrt = rt6_alloc_cow(rt,&fl6->daddr,&fl6->saddr); > else if (!(rt->dst.flags& DST_HOST)) > nrt = rt6_alloc_clone(rt,&fl6->daddr); > @@ -1587,7 +1596,7 @@ void rt6_redirect(const struct in6_addr *dest, const struct in6_addr *src, > dst_confirm(&rt->dst); > > /* Duplicate redirect: silently ignore. */ > - if (neigh == dst_get_neighbour(&rt->dst)) > + if (neigh == dst_get_neighbour_raw(&rt->dst)) > goto out; > > nrt = ip6_rt_copy(rt, dest); > @@ -1682,7 +1691,7 @@ again: > 1. It is connected route. Action: COW > 2. It is gatewayed route or NONEXTHOP route. Action: clone it. > */ > - if (!dst_get_neighbour(&rt->dst)&& !(rt->rt6i_flags& RTF_NONEXTHOP)) > + if (!dst_get_neighbour_raw(&rt->dst)&& !(rt->rt6i_flags& RTF_NONEXTHOP)) > nrt = rt6_alloc_cow(rt, daddr, saddr); > else > nrt = rt6_alloc_clone(rt, daddr); > @@ -2326,6 +2335,7 @@ static int rt6_fill_node(struct net *net, > struct nlmsghdr *nlh; > long expires; > u32 table; > + struct neighbour *n; > > if (prefix) { /* user wants prefix routes only */ > if (!(rt->rt6i_flags& RTF_PREFIX_RT)) { > @@ -2414,8 +2424,11 @@ static int rt6_fill_node(struct net *net, > if (rtnetlink_put_metrics(skb, dst_metrics_ptr(&rt->dst))< 0) > goto nla_put_failure; > > - if (dst_get_neighbour(&rt->dst)) > - NLA_PUT(skb, RTA_GATEWAY, 16,&dst_get_neighbour(&rt->dst)->primary_key); > + rcu_read_lock(); > + n = dst_get_neighbour(&rt->dst); > + if (n) > + NLA_PUT(skb, RTA_GATEWAY, 16,&n->primary_key); > + rcu_read_unlock(); > > if (rt->dst.dev) > NLA_PUT_U32(skb, RTA_OIF, rt->rt6i_dev->ifindex); > @@ -2608,12 +2621,14 @@ static int rt6_info_route(struct rt6_info *rt, void *p_arg) > #else > seq_puts(m, "00000000000000000000000000000000 00 "); > #endif > + rcu_read_lock(); > n = dst_get_neighbour(&rt->dst); > if (n) { > seq_printf(m, "%pi6", n->primary_key); > } else { > seq_puts(m, "00000000000000000000000000000000"); > } > + rcu_read_unlock(); > seq_printf(m, " %08x %08x %08x %08x %8s\n", > rt->rt6i_metric, atomic_read(&rt->dst.__refcnt), > rt->dst.__use, rt->rt6i_flags, > -- 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
Powered by blists - more mailing lists