allowedips: prevent double read in kref

Blocks like:

    if (node_placement(*trie, key, cidr, bits, &node, lock)) {
        node->peer = peer;
        return 0;
    }

May result in a double read when adjusting the refcount, in the highly
unlikely case of LTO and an overly smart compiler.

While we're at it, replace rcu_assign_pointer(X, NULL); with
RCU_INIT_POINTER.

Reported-by: Jann Horn <jann@thejh.net>

2 files changed, 17 insertions, 17 deletions

diff --git a/src/allowedips.c b/src/allowedips.c
index aecb390..d545be4 100644
--- a/src/allowedips.c
+++ b/src/allowedips.c
@@ -7,7 +7,7 @@
 #include "peer.h"
 
 struct allowedips_node {
-	struct wireguard_peer *peer;
+	struct wireguard_peer __rcu *peer;
 	struct rcu_head rcu;
 	struct allowedips_node __rcu *bit[2];
 	/* While it may seem scandalous that we waste space for v4,
@@ -75,7 +75,7 @@ static int walk_by_peer(struct allowedips_node __rcu *top, u8 bits, struct allow
 		push(cursor->stack, top, cursor->len);
 
 	for (; cursor->len > 0 && (node = cursor->stack[cursor->len - 1]); --cursor->len, push(cursor->stack, node->bit[0], cursor->len), push(cursor->stack, node->bit[1], cursor->len)) {
-		if (node->peer != peer)
+		if (rcu_dereference_protected(node->peer, lockdep_is_held(lock)) != peer)
 			continue;
 
 		swap_endian(ip, node->bits, bits);
@@ -119,8 +119,8 @@ static void walk_remove_by_peer(struct allowedips_node __rcu **top, struct wireg
 			if (ref(node->bit[1]))
 				push(&node->bit[1]);
 		} else {
-			if (node->peer == peer) {
-				node->peer = NULL;
+			if (rcu_dereference_protected(node->peer, lockdep_is_held(lock)) == peer) {
+				RCU_INIT_POINTER(node->peer, NULL);
 				if (!node->bit[0] || !node->bit[1]) {
 					rcu_assign_pointer(*nptr, deref(&node->bit[!ref(node->bit[0])]));
 					call_rcu_bh(&node->rcu, node_free_rcu);
@@ -161,7 +161,7 @@ static __always_inline struct allowedips_node *find_node(struct allowedips_node
 	struct allowedips_node *node = trie, *found = NULL;
 
 	while (node && prefix_matches(node, key, bits)) {
-		if (node->peer)
+		if (rcu_access_pointer(node->peer))
 			found = node;
 		if (node->cidr == bits)
 			break;
@@ -182,7 +182,7 @@ static __always_inline struct wireguard_peer *lookup(struct allowedips_node __rc
 	rcu_read_lock_bh();
 	node = find_node(rcu_dereference_bh(root), bits, ip);
 	if (node)
-		peer = peer_get_maybe_zero(node->peer);
+		peer = peer_get_maybe_zero(rcu_dereference_bh(node->peer));
 	rcu_read_unlock_bh();
 	return peer;
 }
@@ -219,20 +219,20 @@ static int add(struct allowedips_node __rcu **trie, u8 bits, const u8 *be_key, u
 		node = kzalloc(sizeof(*node), GFP_KERNEL);
 		if (!node)
 			return -ENOMEM;
-		node->peer = peer;
+		RCU_INIT_POINTER(node->peer, peer);
 		copy_and_assign_cidr(node, key, cidr, bits);
 		rcu_assign_pointer(*trie, node);
 		return 0;
 	}
 	if (node_placement(*trie, key, cidr, bits, &node, lock)) {
-		node->peer = peer;
+		rcu_assign_pointer(node->peer, peer);
 		return 0;
 	}
 
 	newnode = kzalloc(sizeof(*newnode), GFP_KERNEL);
 	if (!newnode)
 		return -ENOMEM;
-	newnode->peer = peer;
+	RCU_INIT_POINTER(newnode->peer, peer);
 	copy_and_assign_cidr(newnode, key, cidr, bits);
 
 	if (!node)
@@ -281,8 +281,8 @@ void allowedips_free(struct allowedips *table, struct mutex *lock)
 {
 	struct allowedips_node __rcu *old4 = table->root4, *old6 = table->root6;
 	++table->seq;
-	rcu_assign_pointer(table->root4, NULL);
-	rcu_assign_pointer(table->root6, NULL);
+	RCU_INIT_POINTER(table->root4, NULL);
+	RCU_INIT_POINTER(table->root6, NULL);
 	free_root_node(old4, lock);
 	free_root_node(old6, lock);
 }
diff --git a/src/noise.c b/src/noise.c
index 1a28b47..c7a55e0 100644
--- a/src/noise.c
+++ b/src/noise.c
@@ -138,13 +138,13 @@ void noise_keypairs_clear(struct noise_keypairs *keypairs)
 
 	spin_lock_bh(&keypairs->keypair_update_lock);
 	old = rcu_dereference_protected(keypairs->previous_keypair, lockdep_is_held(&keypairs->keypair_update_lock));
-	rcu_assign_pointer(keypairs->previous_keypair, NULL);
+	RCU_INIT_POINTER(keypairs->previous_keypair, NULL);
 	noise_keypair_put(old);
 	old = rcu_dereference_protected(keypairs->next_keypair, lockdep_is_held(&keypairs->keypair_update_lock));
-	rcu_assign_pointer(keypairs->next_keypair, NULL);
+	RCU_INIT_POINTER(keypairs->next_keypair, NULL);
 	noise_keypair_put(old);
 	old = rcu_dereference_protected(keypairs->current_keypair, lockdep_is_held(&keypairs->keypair_update_lock));
-	rcu_assign_pointer(keypairs->current_keypair, NULL);
+	RCU_INIT_POINTER(keypairs->current_keypair, NULL);
 	noise_keypair_put(old);
 	spin_unlock_bh(&keypairs->keypair_update_lock);
 }
@@ -169,7 +169,7 @@ static void add_new_keypair(struct noise_keypairs *keypairs, struct noise_keypai
 			 * next keypair instead of putting it in the previous slot, but this
 			 * might be a bit less robust. Something to think about and decide on.
 			 */
-			rcu_assign_pointer(keypairs->next_keypair, NULL);
+			RCU_INIT_POINTER(keypairs->next_keypair, NULL);
 			rcu_assign_pointer(keypairs->previous_keypair, next_keypair);
 			noise_keypair_put(current_keypair);
 		} else	/* If there wasn't an existing next keypair, we replace the
@@ -189,7 +189,7 @@ static void add_new_keypair(struct noise_keypairs *keypairs, struct noise_keypai
 		 */
 		rcu_assign_pointer(keypairs->next_keypair, new_keypair);
 		noise_keypair_put(next_keypair);
-		rcu_assign_pointer(keypairs->previous_keypair, NULL);
+		RCU_INIT_POINTER(keypairs->previous_keypair, NULL);
 		noise_keypair_put(previous_keypair);
 	}
 	spin_unlock_bh(&keypairs->keypair_update_lock);
@@ -220,7 +220,7 @@ bool noise_received_with_keypair(struct noise_keypairs *keypairs, struct noise_k
 	rcu_assign_pointer(keypairs->previous_keypair, rcu_dereference_protected(keypairs->current_keypair, lockdep_is_held(&keypairs->keypair_update_lock)));
 	noise_keypair_put(old_keypair);
 	rcu_assign_pointer(keypairs->current_keypair, received_keypair);
-	rcu_assign_pointer(keypairs->next_keypair, NULL);
+	RCU_INIT_POINTER(keypairs->next_keypair, NULL);
 
 	spin_unlock_bh(&keypairs->keypair_update_lock);
 	return true;