[PATCH 2/2] dm btree: reject a node whose value size is not the reading level's

From: Bryam Vargas <[email protected]>

value_ptr() takes an entry's address from the value_size on disk while
the caller supplies the length it copies; a validator cannot compare the
two, since it sees the block and never the caller. A leaf can pass every
check the previous patch adds and still be laid out for a narrower
value, leaving insert_at() and btree_split_beneath() striding with the
caller's size over a base placed by the node's -- writes that land
kilobytes past the block.

Compare the sizes wherever both are in hand. Everything that modifies a
node reaches it through bn_shadow(), so one check there covers insert,
the splits, remove and the space map's refcount overflow leaf; the
readers and the two paths that bypass the spine take it where the
expected size is known. A node is only ever read through the level that
wrote it, so conforming metadata is unaffected.

Fixes: 3241b1d3e0aa ("dm: add persistent data library")
Cc: [email protected]
Signed-off-by: Bryam Vargas <[email protected]>
---
 drivers/md/persistent-data/dm-btree-internal.h |  6 +-
 drivers/md/persistent-data/dm-btree-remove.c   | 24 ++++++--
 drivers/md/persistent-data/dm-btree-spine.c    | 13 ++++-
 drivers/md/persistent-data/dm-btree.c          | 79 +++++++++++++++++++++++---
 4 files changed, 108 insertions(+), 14 deletions(-)

diff --git a/drivers/md/persistent-data/dm-btree-internal.h b/drivers/md/persistent-data/dm-btree-internal.h
index 404739149d02..7d5741572b16 100644
--- a/drivers/md/persistent-data/dm-btree-internal.h
+++ b/drivers/md/persistent-data/dm-btree-internal.h
@@ -51,8 +51,10 @@ uint32_t calc_max_entries(size_t value_size, size_t block_size);
 int bn_read_lock(struct dm_btree_info *info, dm_block_t b,
 		 struct dm_block **result);
 
-void inc_children(struct dm_transaction_manager *tm, struct btree_node *n,
-		  struct dm_btree_value_type *vt);
+int check_value_size(struct btree_node *n, size_t expected);
+
+int inc_children(struct dm_transaction_manager *tm, struct btree_node *n,
+		 struct dm_btree_value_type *vt);
 
 int new_block(struct dm_btree_info *info, struct dm_block **result);
 void unlock_block(struct dm_btree_info *info, struct dm_block *b);
diff --git a/drivers/md/persistent-data/dm-btree-remove.c b/drivers/md/persistent-data/dm-btree-remove.c
index aeec5b9a1dd5..64f3313f5a1c 100644
--- a/drivers/md/persistent-data/dm-btree-remove.c
+++ b/drivers/md/persistent-data/dm-btree-remove.c
@@ -175,8 +175,14 @@ static int init_child(struct dm_btree_info *info, struct dm_btree_value_type *vt
 
 	result->n = dm_block_data(result->block);
 
-	if (inc)
-		inc_children(info->tm, result->n, vt);
+	r = check_value_size(result->n, vt->size);
+	if (!r && inc)
+		r = inc_children(info->tm, result->n, vt);
+
+	if (r) {
+		dm_tm_unlock(info->tm, result->block);
+		return r;
+	}
 
 	*((__le64 *) value_ptr(parent, index)) =
 		cpu_to_le64(dm_block_location(result->block));
@@ -501,8 +507,18 @@ static int rebalance_children(struct shadow_spine *s,
 		if (r)
 			return r;
 
-		if (is_shared)
-			inc_children(info->tm, dm_block_data(child), vt);
+		/*
+		 * The child is copied over the node the spine already
+		 * checked, so it has to answer for itself first.
+		 */
+		r = check_value_size(dm_block_data(child), vt->size);
+		if (!r && is_shared)
+			r = inc_children(info->tm, dm_block_data(child), vt);
+
+		if (r) {
+			dm_tm_unlock(info->tm, child);
+			return r;
+		}
 
 		memcpy(n, dm_block_data(child),
 		       dm_bm_block_size(dm_tm_get_bm(info->tm)));
diff --git a/drivers/md/persistent-data/dm-btree-spine.c b/drivers/md/persistent-data/dm-btree-spine.c
index 076f836912c8..2527af9460fe 100644
--- a/drivers/md/persistent-data/dm-btree-spine.c
+++ b/drivers/md/persistent-data/dm-btree-spine.c
@@ -121,8 +121,19 @@ static int bn_shadow(struct dm_btree_info *info, dm_block_t orig,
 
 	r = dm_tm_shadow_block(info->tm, orig, &btree_node_validator,
 			       result, &inc);
+	if (r)
+		return r;
+
+	/*
+	 * Everything that modifies a node reaches it through here, so this is
+	 * where a leaf laid out for a different value size is caught.
+	 */
+	r = check_value_size(dm_block_data(*result), vt->size);
 	if (!r && inc)
-		inc_children(info->tm, dm_block_data(*result), vt);
+		r = inc_children(info->tm, dm_block_data(*result), vt);
+
+	if (r)
+		unlock_block(info, *result);
 
 	return r;
 }
diff --git a/drivers/md/persistent-data/dm-btree.c b/drivers/md/persistent-data/dm-btree.c
index 5ed3b3e9abb9..b74c41fe6213 100644
--- a/drivers/md/persistent-data/dm-btree.c
+++ b/drivers/md/persistent-data/dm-btree.c
@@ -71,16 +71,47 @@ static int upper_bound(struct btree_node *n, uint64_t key)
 	return bsearch(n, key, 1);
 }
 
-void inc_children(struct dm_transaction_manager *tm, struct btree_node *n,
-		  struct dm_btree_value_type *vt)
+/*
+ * value_ptr() takes an entry's address from the value_size stored on disk,
+ * but the caller supplies the length it copies.  node_check() sees the block
+ * and never the caller, so the two are compared here, wherever both are in
+ * hand.  Internal nodes are exempt: node_check() already holds them to
+ * sizeof(__le64), which is what value64() assumes.
+ */
+int check_value_size(struct btree_node *n, size_t expected)
+{
+	uint32_t value_size = le32_to_cpu(n->header.value_size);
+
+	if (le32_to_cpu(n->header.flags) & INTERNAL_NODE)
+		return 0;
+
+	if (value_size != expected) {
+		DMERR_LIMIT("%s failed: value_size %u != %zu expected by the caller",
+			    __func__, value_size, expected);
+		return -EILSEQ;
+	}
+
+	return 0;
+}
+
+int inc_children(struct dm_transaction_manager *tm, struct btree_node *n,
+		 struct dm_btree_value_type *vt)
 {
 	uint32_t nr_entries = le32_to_cpu(n->header.nr_entries);
 
 	if (le32_to_cpu(n->header.flags) & INTERNAL_NODE)
 		dm_tm_with_runs(tm, value_ptr(n, 0), nr_entries, dm_tm_inc_range);
 
-	else if (vt->inc)
+	else if (vt->inc) {
+		int r = check_value_size(n, vt->size);
+
+		if (r)
+			return r;
+
 		vt->inc(vt->context, value_ptr(n, 0), nr_entries);
+	}
+
+	return 0;
 }
 
 static int insert_at(size_t value_size, struct btree_node *node, unsigned int index,
@@ -314,6 +345,10 @@ int dm_btree_del(struct dm_btree_info *info, dm_block_t root)
 				goto out;
 
 		} else if (is_internal_level(info, f)) {
+			r = check_value_size(f->n, sizeof(__le64));
+			if (r)
+				goto out;
+
 			b = value64(f->n, f->current_child);
 			f->current_child++;
 			r = push_frame(s, b, f->level + 1);
@@ -321,9 +356,14 @@ int dm_btree_del(struct dm_btree_info *info, dm_block_t root)
 				goto out;
 
 		} else {
-			if (info->value_type.dec)
+			if (info->value_type.dec) {
+				r = check_value_size(f->n, info->value_type.size);
+				if (r)
+					goto out;
+
 				info->value_type.dec(info->value_type.context,
 						     value_ptr(f->n, 0), f->nr_children);
+			}
 			pop_frame(s);
 		}
 	}
@@ -365,8 +405,13 @@ static int btree_lookup_raw(struct ro_spine *s, dm_block_t block, uint64_t key,
 	} while (!(flags & LEAF_NODE));
 
 	*result_key = le64_to_cpu(ro_node(s)->keys[i]);
-	if (v)
+	if (v) {
+		r = check_value_size(ro_node(s), value_size);
+		if (r)
+			return r;
+
 		memcpy(v, value_ptr(ro_node(s), i), value_size);
+	}
 
 	return 0;
 }
@@ -460,6 +505,10 @@ static int dm_btree_lookup_next_single(struct dm_btree_info *info, dm_block_t ro
 		}
 
 		*rkey = le64_to_cpu(n->keys[i]);
+		r = check_value_size(n, info->value_type.size);
+		if (r)
+			goto out;
+
 		memcpy(value_le, value_ptr(n, i), info->value_type.size);
 	}
 out:
@@ -721,8 +770,14 @@ static int shadow_child(struct dm_btree_info *info, struct dm_btree_value_type *
 
 	node = dm_block_data(*result);
 
-	if (inc)
-		inc_children(info->tm, node, vt);
+	r = check_value_size(node, vt->size);
+	if (!r && inc)
+		r = inc_children(info->tm, node, vt);
+
+	if (r) {
+		unlock_block(info, *result);
+		return r;
+	}
 
 	*((__le64 *) value_ptr(parent, index)) =
 		cpu_to_le64(dm_block_location(*result));
@@ -1441,6 +1496,10 @@ static int walk_node(struct dm_btree_info *info, dm_block_t block,
 			if (r)
 				goto out;
 		} else {
+			r = check_value_size(n, info->value_type.size);
+			if (r)
+				goto out;
+
 			keys = le64_to_cpu(*key_ptr(n, i));
 			r = fn(context, &keys, value_ptr(n, i));
 			if (r)
@@ -1474,6 +1533,9 @@ static void prefetch_values(struct dm_btree_cursor *c)
 
 	BUG_ON(c->info->value_type.size != sizeof(value_le));
 
+	if (check_value_size(bn, sizeof(value_le)))
+		return;
+
 	nr = le32_to_cpu(bn->header.nr_entries);
 	for (i = 0; i < nr; i++) {
 		memcpy(&value_le, value_ptr(bn, i), sizeof(value_le));
@@ -1627,6 +1689,9 @@ int dm_btree_cursor_get_value(struct dm_btree_cursor *c, uint64_t *key, void *va
 		if (le32_to_cpu(bn->header.flags) & INTERNAL_NODE)
 			return -EINVAL;
 
+		if (check_value_size(bn, c->info->value_type.size))
+			return -EILSEQ;
+
 		*key = le64_to_cpu(*key_ptr(bn, n->index));
 		memcpy(value_le, value_ptr(bn, n->index), c->info->value_type.size);
 		return 0;

-- 
2.55.0