linux-kernel - [PATCH 19/38] drbd: fix NULL deref in remember_new

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: <1448448851-10343-20-git-send-email-philipp.reisner@linbit.com>
Date:	Wed, 25 Nov 2015 11:53:52 +0100
From:	Philipp Reisner <philipp.reisner@...bit.com>
To:	Jens Axboe <axboe@...com>, linux-kernel@...r.kernel.org
Cc:	drbd-dev@...ts.linbit.com
Subject: [PATCH 19/38] drbd: fix NULL deref in remember_new_state

From: Lars Ellenberg <lars.ellenberg@...bit.com>

The recent (not yet released) backport of the extended state broadcasts
to support the "events2" subcommand of drbdsetup had some glitches.

remember_old_state() would first count all connections with a
net_conf != NULL, then allocate a suitable array, then populate that
array with all connections found to have net_conf != NULL.

This races with the state change to C_STANDALONE,
and the NULL assignment there.

remember_new_state() then iterates over said connection array,
assuming that it would be fully populated.

But rcu_lock() just makes sure the thing some pointer points to,
if any, won't go away. It does not make the pointer itself immutable.

In fact there is no need to "filter" connections based on whether or not
they have a currently valid configuration.  Just record them always, if
they don't have a config, that's fine, there will be no change then.

Signed-off-by: Philipp Reisner <philipp.reisner@...bit.com>
Signed-off-by: Lars Ellenberg <lars.ellenberg@...bit.com>
---
 drivers/block/drbd/drbd_state.c | 46 +++++++++++++----------------------------
 1 file changed, 14 insertions(+), 32 deletions(-)

diff --git a/drivers/block/drbd/drbd_state.c b/drivers/block/drbd/drbd_state.c
index a4e4505..f022e99 100644
--- a/drivers/block/drbd/drbd_state.c
+++ b/drivers/block/drbd/drbd_state.c
@@ -63,11 +63,8 @@ static void count_objects(struct drbd_resource *resource,
 
 	idr_for_each_entry(&resource->devices, device, vnr)
 		(*n_devices)++;
-	for_each_connection(connection, resource) {
-		if (!has_net_conf(connection))
-			continue;
+	for_each_connection(connection, resource)
 		(*n_connections)++;
-	}
 }
 
 static struct drbd_state_change *alloc_state_change(unsigned int n_devices, unsigned int n_connections, gfp_t gfp)
@@ -108,23 +105,13 @@ struct drbd_state_change *remember_old_state(struct drbd_resource *resource, gfp
 	struct drbd_peer_device_state_change *peer_device_state_change;
 	struct drbd_connection_state_change *connection_state_change;
 
-retry:
-	rcu_read_lock();
+	/* Caller holds req_lock spinlock.
+	 * No state, no device IDR, no connections lists can change. */
 	count_objects(resource, &n_devices, &n_connections);
-	rcu_read_unlock();
 	state_change = alloc_state_change(n_devices, n_connections, gfp);
 	if (!state_change)
 		return NULL;
 
-	rcu_read_lock();
-	count_objects(resource, &n_devices, &n_connections);
-	if (n_devices != state_change->n_devices ||
-	    n_connections != state_change->n_connections) {
-		kfree(state_change);
-		rcu_read_unlock();
-		goto retry;
-	}
-
 	kref_get(&resource->kref);
 	state_change->resource->resource = resource;
 	state_change->resource->role[OLD] =
@@ -133,6 +120,17 @@ retry:
 	state_change->resource->susp_nod[OLD] = resource->susp_nod;
 	state_change->resource->susp_fen[OLD] = resource->susp_fen;
 
+	connection_state_change = state_change->connections;
+	for_each_connection(connection, resource) {
+		kref_get(&connection->kref);
+		connection_state_change->connection = connection;
+		connection_state_change->cstate[OLD] =
+			connection->cstate;
+		connection_state_change->peer_role[OLD] =
+			conn_highest_peer(connection);
+		connection_state_change++;
+	}
+
 	device_state_change = state_change->devices;
 	peer_device_state_change = state_change->peer_devices;
 	idr_for_each_entry(&resource->devices, device, vnr) {
@@ -145,8 +143,6 @@ retry:
 		for_each_connection(connection, resource) {
 			struct drbd_peer_device *peer_device;
 
-			if (!has_net_conf(connection))
-				continue;
 			peer_device = conn_peer_device(connection, device->vnr);
 			peer_device_state_change->peer_device = peer_device;
 			peer_device_state_change->disk_state[OLD] =
@@ -165,20 +161,6 @@ retry:
 		device_state_change++;
 	}
 
-	connection_state_change = state_change->connections;
-	for_each_connection(connection, resource) {
-		if (!has_net_conf(connection))
-			continue;
-		kref_get(&connection->kref);
-		connection_state_change->connection = connection;
-		connection_state_change->cstate[OLD] =
-			connection->cstate;
-		connection_state_change->peer_role[OLD] =
-			conn_highest_peer(connection);
-		connection_state_change++;
-	}
-	rcu_read_unlock();
-
 	return state_change;
 }
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/