Skip to content

Commit 92f1655

Browse files
edumazetkuba-moo
authored andcommitted
net: fix __dst_negative_advice() race
__dst_negative_advice() does not enforce proper RCU rules when sk->dst_cache must be cleared, leading to possible UAF. RCU rules are that we must first clear sk->sk_dst_cache, then call dst_release(old_dst). Note that sk_dst_reset(sk) is implementing this protocol correctly, while __dst_negative_advice() uses the wrong order. Given that ip6_negative_advice() has special logic against RTF_CACHE, this means each of the three ->negative_advice() existing methods must perform the sk_dst_reset() themselves. Note the check against NULL dst is centralized in __dst_negative_advice(), there is no need to duplicate it in various callbacks. Many thanks to Clement Lecigne for tracking this issue. This old bug became visible after the blamed commit, using UDP sockets. Fixes: a87cb3e ("net: Facility to report route quality of connected sockets") Reported-by: Clement Lecigne <[email protected]> Diagnosed-by: Clement Lecigne <[email protected]> Signed-off-by: Eric Dumazet <[email protected]> Cc: Tom Herbert <[email protected]> Reviewed-by: David Ahern <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jakub Kicinski <[email protected]>
1 parent 068648a commit 92f1655

File tree

5 files changed

+30
-47
lines changed

5 files changed

+30
-47
lines changed

include/net/dst_ops.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ struct dst_ops {
2424
void (*destroy)(struct dst_entry *);
2525
void (*ifdown)(struct dst_entry *,
2626
struct net_device *dev);
27-
struct dst_entry * (*negative_advice)(struct dst_entry *);
27+
void (*negative_advice)(struct sock *sk, struct dst_entry *);
2828
void (*link_failure)(struct sk_buff *);
2929
void (*update_pmtu)(struct dst_entry *dst, struct sock *sk,
3030
struct sk_buff *skb, u32 mtu,

include/net/sock.h

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2063,17 +2063,10 @@ sk_dst_get(const struct sock *sk)
20632063

20642064
static inline void __dst_negative_advice(struct sock *sk)
20652065
{
2066-
struct dst_entry *ndst, *dst = __sk_dst_get(sk);
2066+
struct dst_entry *dst = __sk_dst_get(sk);
20672067

2068-
if (dst && dst->ops->negative_advice) {
2069-
ndst = dst->ops->negative_advice(dst);
2070-
2071-
if (ndst != dst) {
2072-
rcu_assign_pointer(sk->sk_dst_cache, ndst);
2073-
sk_tx_queue_clear(sk);
2074-
WRITE_ONCE(sk->sk_dst_pending_confirm, 0);
2075-
}
2076-
}
2068+
if (dst && dst->ops->negative_advice)
2069+
dst->ops->negative_advice(sk, dst);
20772070
}
20782071

20792072
static inline void dst_negative_advice(struct sock *sk)

net/ipv4/route.c

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,8 @@ struct dst_entry *ipv4_dst_check(struct dst_entry *dst, u32 cookie);
129129
static unsigned int ipv4_default_advmss(const struct dst_entry *dst);
130130
INDIRECT_CALLABLE_SCOPE
131131
unsigned int ipv4_mtu(const struct dst_entry *dst);
132-
static struct dst_entry *ipv4_negative_advice(struct dst_entry *dst);
132+
static void ipv4_negative_advice(struct sock *sk,
133+
struct dst_entry *dst);
133134
static void ipv4_link_failure(struct sk_buff *skb);
134135
static void ip_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
135136
struct sk_buff *skb, u32 mtu,
@@ -825,22 +826,15 @@ static void ip_do_redirect(struct dst_entry *dst, struct sock *sk, struct sk_buf
825826
__ip_do_redirect(rt, skb, &fl4, true);
826827
}
827828

828-
static struct dst_entry *ipv4_negative_advice(struct dst_entry *dst)
829+
static void ipv4_negative_advice(struct sock *sk,
830+
struct dst_entry *dst)
829831
{
830832
struct rtable *rt = dst_rtable(dst);
831-
struct dst_entry *ret = dst;
832833

833-
if (rt) {
834-
if (dst->obsolete > 0) {
835-
ip_rt_put(rt);
836-
ret = NULL;
837-
} else if ((rt->rt_flags & RTCF_REDIRECTED) ||
838-
rt->dst.expires) {
839-
ip_rt_put(rt);
840-
ret = NULL;
841-
}
842-
}
843-
return ret;
834+
if ((dst->obsolete > 0) ||
835+
(rt->rt_flags & RTCF_REDIRECTED) ||
836+
rt->dst.expires)
837+
sk_dst_reset(sk);
844838
}
845839

846840
/*

net/ipv6/route.c

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,8 @@ struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie);
8787
static unsigned int ip6_default_advmss(const struct dst_entry *dst);
8888
INDIRECT_CALLABLE_SCOPE
8989
unsigned int ip6_mtu(const struct dst_entry *dst);
90-
static struct dst_entry *ip6_negative_advice(struct dst_entry *);
90+
static void ip6_negative_advice(struct sock *sk,
91+
struct dst_entry *dst);
9192
static void ip6_dst_destroy(struct dst_entry *);
9293
static void ip6_dst_ifdown(struct dst_entry *,
9394
struct net_device *dev);
@@ -2770,24 +2771,24 @@ INDIRECT_CALLABLE_SCOPE struct dst_entry *ip6_dst_check(struct dst_entry *dst,
27702771
}
27712772
EXPORT_INDIRECT_CALLABLE(ip6_dst_check);
27722773

2773-
static struct dst_entry *ip6_negative_advice(struct dst_entry *dst)
2774+
static void ip6_negative_advice(struct sock *sk,
2775+
struct dst_entry *dst)
27742776
{
27752777
struct rt6_info *rt = dst_rt6_info(dst);
27762778

2777-
if (rt) {
2778-
if (rt->rt6i_flags & RTF_CACHE) {
2779-
rcu_read_lock();
2780-
if (rt6_check_expired(rt)) {
2781-
rt6_remove_exception_rt(rt);
2782-
dst = NULL;
2783-
}
2784-
rcu_read_unlock();
2785-
} else {
2786-
dst_release(dst);
2787-
dst = NULL;
2779+
if (rt->rt6i_flags & RTF_CACHE) {
2780+
rcu_read_lock();
2781+
if (rt6_check_expired(rt)) {
2782+
/* counteract the dst_release() in sk_dst_reset() */
2783+
dst_hold(dst);
2784+
sk_dst_reset(sk);
2785+
2786+
rt6_remove_exception_rt(rt);
27882787
}
2788+
rcu_read_unlock();
2789+
return;
27892790
}
2790-
return dst;
2791+
sk_dst_reset(sk);
27912792
}
27922793

27932794
static void ip6_link_failure(struct sk_buff *skb)

net/xfrm/xfrm_policy.c

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3910,15 +3910,10 @@ static void xfrm_link_failure(struct sk_buff *skb)
39103910
/* Impossible. Such dst must be popped before reaches point of failure. */
39113911
}
39123912

3913-
static struct dst_entry *xfrm_negative_advice(struct dst_entry *dst)
3913+
static void xfrm_negative_advice(struct sock *sk, struct dst_entry *dst)
39143914
{
3915-
if (dst) {
3916-
if (dst->obsolete) {
3917-
dst_release(dst);
3918-
dst = NULL;
3919-
}
3920-
}
3921-
return dst;
3915+
if (dst->obsolete)
3916+
sk_dst_reset(sk);
39223917
}
39233918

39243919
static void xfrm_init_pmtu(struct xfrm_dst **bundle, int nr)

0 commit comments

Comments
 (0)