[PATCH v2 0/2] dm array: validate array block headers, and against the caller

[PATCH v2 0/2] dm array: validate array block headers, and against the caller

[Bryam Vargas via B4 Relay]

v1 fixed one of these and mentioned the other; this is both, and the second one
is here because Ming-Hung Tsai said how to reproduce it.

An array block header can be wrong in two independent ways, and a validator can
only catch the first. 1/2 makes the header consistent with itself: value_size
non-zero, max_entries exactly calc_max_entries() for that value_size and block
size, nr_entries within it. 2/2 catches the header that is perfectly consistent
with itself and still wrong for the array reading it.

The review of v1 [1] asked for three changes to the first, all taken: compare
max_entries for equality rather than bounding it from above, since a count
BELOW the real capacity reaches BUG_ON(new_nr > ab->max_entries) in
fill_ablock() and trim_ablock(); split the !value_size test out with its own
message; and print
the counts rather than a bare "max_entries too large". calc_max_entries() moved
above the validators so it is in scope there -- it is unchanged, and
open-coding the formula was what I wanted to avoid, since max_entries is what
dm_array_get_value() addresses with.

2/2 is the hole v1 described and did not fix. dm-cache keeps two arrays,
mappings at 8 bytes and hints at 4, and both roots sit in the superblock. A
hint block stores value_size 4 and max_entries 1018, and 24 + 4*1018 is exactly
4096, so it is internally consistent and 1/2 passes it -- the validator sees the
block, never the caller. Swap the two roots and __load_mappings() walks the hint
array through an info whose value size is 8. get_ablock() and __shadow_ablock()
are the two readers that hold the block and the caller at once, which is why the
check has to be in both; covering only one is why this was not in v1.

A/B on v7.2-rc1 with KASAN; dm-array.c, dm-bufio.c and dm-cache-metadata.c are
identical between that and the base of this series. dm-persistent-data is a
module here, so an arm is the .ko that is loaded and the tested code carries no
scaffolding. Every image is derived from metadata dm-cache itself wrote:

  pristine,                  unpatched:  activates, loads its mappings
  nr_entries 0xFFFFFFFF,     unpatched:  reads past the block, below
  max_entries 100,           unpatched:  ACTIVATES CLEAN -- accepted today
  roots swapped,             unpatched:  reads past the block, below
  nr_entries 0xFFFFFFFF,     patched:    "too many entries", -EILSEQ
  max_entries 100,           patched:    "max_entries 100 != wanted 509 for value_size 8"
  roots swapped,             patched:    "get_ablock failed: value_size 4 != wanted 8"
  pristine,                  patched:    activates, dmesg empty

Three crafted images, three different rejections, and the real image still
loads. The two overread arms, with the consequence dm-cache reports on its own:

    device-mapper: cache: 252:0: unable to shrink origin; cache block 509 is dirty
    device-mapper: cache metadata: policy couldn't load cache block 509     (nr_entries)

    device-mapper: cache: 252:0: unable to shrink origin; cache block 911 is dirty
    device-mapper: cache metadata: policy couldn't load cache block 911     (roots swapped)

509 is exactly the first index an 8-byte-value block cannot hold. 911 is only
where the swapped-root walk first decodes a mapping dm-cache refuses; it depends
on the data, not on a boundary.

Those two lines are the oracle. KASAN adds little here. One run of the
nr_entries arm reported

    BUG: KASAN: slab-use-after-free in dm_cache_load_mappings+0x9b1/0xb40 [dm_cache]

but that build carried an extra warning I had put at the read site to locate it,
and no run since has reported anything. dm-bufio serves these buffers from
__get_free_pages(), the page allocator has no redzones, and the first byte past
the block is the first byte of the next page, so KASAN only speaks when that
page happens to be poisoned.

Scope of the max_entries arm, since it is weaker than the other two: it shows
the header is accepted today and rejected with 1/2, which is what the equality
is for. It does not show the BUG_ON firing -- reads bound themselves by
nr_entries, so a too-small max_entries only bites on a later resize, and I did
not drive one. By inspection from fill_ablock() and trim_ablock().

Forging any of this costs nothing: dm_bm_checksum() is crc32c(~0, data, len) ^
xor, a corruption check rather than a tag. Each crafted image differs from the
one the kernel wrote by four or sixteen bytes plus a checksum.

All of it needs CAP_SYS_ADMIN to load the table, which is why this is hardening
rather than a security report. dm-era reaches the same accessor directly and
dm-clone reaches it through dm-bitset; dm-cache is only the shortest path from a
crafted image to an observable read. dm-thin uses neither. cache_check and
era_check have caught the value_size case offline since v1.0; this is the online
half.

[1] https://lore.kernel.org/all/CALjSBEt-PGxY7y6tQYx6iCmFRXqVcwr633FoFsE+L13A=gre4Q@mail.gmail.com/

---
Bryam Vargas (2):
      dm array: validate array block headers on read
      dm array: reject an array block whose value size is not the caller's

 drivers/md/persistent-data/dm-array.c | 54 +++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 8 deletions(-)
---
base-commit: 8ba098e6b6ff0db8edf28528d1552be261af30d4
change-id: 20260731-b4-disp-4fd3bed9-387c3fe87c42

Best regards,
--  
Bryam Vargas <hexlabsecurity@proton.me>

[PATCH v2 1/2] dm array: validate array block headers on read

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

array_block_check() validates blocknr and csum and nothing else, while
node_check(), next to it, has bounded the structural fields since both
were written. dm_array_cursor_next() takes its loop bound from the
on-disk nr_entries and element_at() is unguarded pointer arithmetic, so
a count larger than the block holds keeps the cursor in one block while
the index grows past it and the read walks off the dm-bufio buffer --
dm_cache_load_mappings() drives it once per cache block at activation.

Check the header against itself: reject a zero value_size, require
max_entries to equal calc_max_entries() for that value_size and block
size, and require nr_entries to fit. Equality rather than an upper bound,
since a count below the real capacity trips BUG_ON() in fill_ablock() and
trim_ablock(). Metadata dm-array writes satisfies all three.

Fixes: 6513c29f44f2 ("dm persistent data: add transactional array")
Suggested-by: Ming-Hung Tsai <mtsai@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 drivers/md/persistent-data/dm-array.c | 38 +++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/md/persistent-data/dm-array.c b/drivers/md/persistent-data/dm-array.c
index 8f8792e55806..5949fb0e16c6 100644
--- a/drivers/md/persistent-data/dm-array.c
+++ b/drivers/md/persistent-data/dm-array.c
@@ -38,6 +38,14 @@ struct array_block {
  */
 #define CSUM_XOR 595846735
 
+/*
+ * Each array block can hold this many values.
+ */
+static uint32_t calc_max_entries(size_t value_size, size_t size_of_block)
+{
+	return (size_of_block - sizeof(struct array_block)) / value_size;
+}
+
 static void array_block_prepare_for_write(const struct dm_block_validator *v,
 					  struct dm_block *b,
 					  size_t size_of_block)
@@ -55,6 +63,7 @@ static int array_block_check(const struct dm_block_validator *v,
 			     size_t size_of_block)
 {
 	struct array_block *bh_le = dm_block_data(b);
+	uint32_t nr_entries, max_entries, value_size, wanted;
 	__le32 csum_disk;
 
 	if (dm_block_location(b) != le64_to_cpu(bh_le->blocknr)) {
@@ -74,6 +83,27 @@ static int array_block_check(const struct dm_block_validator *v,
 		return -EILSEQ;
 	}
 
+	nr_entries = le32_to_cpu(bh_le->nr_entries);
+	max_entries = le32_to_cpu(bh_le->max_entries);
+	value_size = le32_to_cpu(bh_le->value_size);
+
+	if (!value_size) {
+		DMERR_LIMIT("%s failed: value_size is zero", __func__);
+		return -EILSEQ;
+	}
+
+	wanted = calc_max_entries(value_size, size_of_block);
+	if (max_entries != wanted) {
+		DMERR_LIMIT("%s failed: max_entries %u != wanted %u for value_size %u",
+			    __func__, max_entries, wanted, value_size);
+		return -EILSEQ;
+	}
+
+	if (nr_entries > max_entries) {
+		DMERR_LIMIT("%s failed: too many entries", __func__);
+		return -EILSEQ;
+	}
+
 	return 0;
 }
 
@@ -138,14 +168,6 @@ static void dec_ablock_entries(struct dm_array_info *info, struct array_block *a
 		on_entries(info, ab, vt->dec);
 }
 
-/*
- * Each array block can hold this many values.
- */
-static uint32_t calc_max_entries(size_t value_size, size_t size_of_block)
-{
-	return (size_of_block - sizeof(struct array_block)) / value_size;
-}
-
 /*
  * Allocate a new array block.  The caller will need to unlock block.
  */

-- 
2.55.0

Re: [PATCH v2 1/2] dm array: validate array block headers on read

[Ming Hung Tsai]

On Sat, Aug 1, 2026 at 6:55 AM Bryam Vargas via B4 Relay
<devnull+hexlabsecurity.proton.me@kernel.org> wrote:
>
> From: Bryam Vargas <hexlabsecurity@proton.me>
>
> array_block_check() validates blocknr and csum and nothing else, while
> node_check(), next to it, has bounded the structural fields since both
> were written. dm_array_cursor_next() takes its loop bound from the
> on-disk nr_entries and element_at() is unguarded pointer arithmetic, so
> a count larger than the block holds keeps the cursor in one block while
> the index grows past it and the read walks off the dm-bufio buffer --
> dm_cache_load_mappings() drives it once per cache block at activation.
>
> Check the header against itself: reject a zero value_size, require
> max_entries to equal calc_max_entries() for that value_size and block
> size, and require nr_entries to fit. Equality rather than an upper bound,
> since a count below the real capacity trips BUG_ON() in fill_ablock() and
> trim_ablock(). Metadata dm-array writes satisfies all three.
>
> Fixes: 6513c29f44f2 ("dm persistent data: add transactional array")
> Suggested-by: Ming-Hung Tsai <mtsai@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Reviewed-by: Ming-Hung Tsai <mtsai@redhat.com>

Reproducer to validate the BUG_ON prevention using crafted metadata:
create a cache device, and reduce the max_entries of the final array
block of the mapping array. Next, expand the fast device to make the
last array block exceeds its capacity, which triggers the BUG_ON in
fill_ablock().

> +       wanted = calc_max_entries(value_size, size_of_block);
> +       if (max_entries != wanted) {
> +               DMERR_LIMIT("%s failed: max_entries %u != wanted %u for value_size %u",
> +                           __func__, max_entries, wanted, value_size);
> +               return -EILSEQ;
> +       }

One minor nit, which needn't hold up the patch: I'd drop "wanted %u"
from the error message. The wanted value is derivable from value_size,
so printing "%s failed: max_entries %u invalid for value_size %u"
carries the same information.

Mikulas, if you agree, feel free to adjust when applying rather than
requesting a v3 patch. Thank you.

Re: [PATCH v2 1/2] dm array: validate array block headers on read

[Bryam Vargas]

On Mon, Aug 03, 2026 at 09:41:36PM +0800, Ming-Hung Tsai wrote:
> One minor nit, which needn't hold up the patch: I'd drop "wanted %u"
> from the error message. The wanted value is derivable from value_size,
> so printing "%s failed: max_entries %u invalid for value_size %u"
> carries the same information.

I'd keep it. The two DMERR_LIMITs above it in array_block_check() are
"blocknr %llu != wanted %llu" and "csum %u != wanted %u", so "%u != wanted
%u" is the shape a reader of this function already expects. You're right
that the wanted value follows from value_size here and doesn't there --
what I was matching is the form, not the information content. Mikulas,
drop it when applying if you'd rather.

> Reproducer to validate the BUG_ON prevention using crafted metadata:
> create a cache device, and reduce the max_entries of the final array
> block of the mapping array. Next, expand the fast device to make the
> last array block exceeds its capacity, which triggers the BUG_ON in
> fill_ablock().

That is the path the equality test is there for -- an upper bound lets a
short max_entries through to fill_ablock() untouched. My A/B covered the
overread only, so that half was by inspection.

Thanks for the review.

Bryam

Re: [PATCH v2 1/2] dm array: validate array block headers on read

[Ming Hung Tsai]

On Sat, Aug 8, 2026 at 5:00 PM Bryam Vargas <hexlabsecurity@proton.me> wrote:
>
> On Mon, Aug 03, 2026 at 09:41:36PM +0800, Ming-Hung Tsai wrote:
> > One minor nit, which needn't hold up the patch: I'd drop "wanted %u"
> > from the error message. The wanted value is derivable from value_size,
> > so printing "%s failed: max_entries %u invalid for value_size %u"
> > carries the same information.
>
> I'd keep it. The two DMERR_LIMITs above it in array_block_check() are
> "blocknr %llu != wanted %llu" and "csum %u != wanted %u", so "%u != wanted
> %u" is the shape a reader of this function already expects. You're right
> that the wanted value follows from value_size here and doesn't there --
> what I was matching is the form, not the information content. Mikulas,
> drop it when applying if you'd rather.

Thanks for the explanation. That's the reason for the shorter form
here: the "wanted" values for csum and blocknr cannot be reproduced
from what is printed, but readers can derive the expected max_entries
from the printed value_size. Mikulas already dropped it from the
staged patches on the dm-next branch.

[PATCH v2 2/2] dm array: reject an array block whose value size is not the caller's

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

array_block_check() can only compare the header against itself, so a block
with value_size 4 and max_entries 1018 is internally consistent and passes.
dm-cache keeps two arrays -- mappings at 8 bytes and hints at 4 -- and the
roots for both live in the superblock. Point the mappings root at a hint
block and __load_mappings() walks it through an info whose value size is 8,
so element_at() strides 8 bytes over 4-byte entries and reaches offset 8160
of a 4096-byte block.

get_ablock() and __shadow_ablock() are the two places that hold the block
and the caller at once. Reject there when the two value sizes disagree.
Arrays only ever read their own blocks, so this fires on crafted metadata
only.

Fixes: 6513c29f44f2 ("dm persistent data: add transactional array")
Suggested-by: Ming-Hung Tsai <mtsai@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 drivers/md/persistent-data/dm-array.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/md/persistent-data/dm-array.c b/drivers/md/persistent-data/dm-array.c
index 5949fb0e16c6..6de1c0467ec9 100644
--- a/drivers/md/persistent-data/dm-array.c
+++ b/drivers/md/persistent-data/dm-array.c
@@ -247,6 +247,14 @@ static int get_ablock(struct dm_array_info *info, dm_block_t b,
 		return r;
 
 	*ab = dm_block_data(*block);
+	if (le32_to_cpu((*ab)->value_size) != info->value_type.size) {
+		DMERR_LIMIT("%s failed: value_size %u != wanted %u", __func__,
+			    le32_to_cpu((*ab)->value_size),
+			    info->value_type.size);
+		dm_tm_unlock(info->btree_info.tm, *block);
+		return -EILSEQ;
+	}
+
 	return 0;
 }
 
@@ -309,6 +317,14 @@ static int __shadow_ablock(struct dm_array_info *info, dm_block_t b,
 		return r;
 
 	*ab = dm_block_data(*block);
+	if (le32_to_cpu((*ab)->value_size) != info->value_type.size) {
+		DMERR_LIMIT("%s failed: value_size %u != wanted %u", __func__,
+			    le32_to_cpu((*ab)->value_size),
+			    info->value_type.size);
+		dm_tm_unlock(info->btree_info.tm, *block);
+		return -EILSEQ;
+	}
+
 	if (inc)
 		inc_ablock_entries(info, *ab);
 

-- 
2.55.0

Re: [PATCH v2 2/2] dm array: reject an array block whose value size is not the caller's

[Ming Hung Tsai]

On Sat, Aug 1, 2026 at 6:55 AM Bryam Vargas via B4 Relay
<devnull+hexlabsecurity.proton.me@kernel.org> wrote:
>
> From: Bryam Vargas <hexlabsecurity@proton.me>
>
> array_block_check() can only compare the header against itself, so a block
> with value_size 4 and max_entries 1018 is internally consistent and passes.
> dm-cache keeps two arrays -- mappings at 8 bytes and hints at 4 -- and the
> roots for both live in the superblock. Point the mappings root at a hint
> block and __load_mappings() walks it through an info whose value size is 8,
> so element_at() strides 8 bytes over 4-byte entries and reaches offset 8160
> of a 4096-byte block.
>
> get_ablock() and __shadow_ablock() are the two places that hold the block
> and the caller at once. Reject there when the two value sizes disagree.
> Arrays only ever read their own blocks, so this fires on crafted metadata
> only.
>
> Fixes: 6513c29f44f2 ("dm persistent data: add transactional array")
> Suggested-by: Ming-Hung Tsai <mtsai@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Reviewed-by: Ming-Hung Tsai <mtsai@redhat.com>

The fix addresses more than crafted metadata. A lost write on disk
drives can cause a stale block that was freed as a hint block,
triggering this out-of-bounds access issue.

Re: [PATCH v2 2/2] dm array: reject an array block whose value size is not the caller's

[Bryam Vargas]

On Mon, Aug 03, 2026 at 10:24:10PM +0800, Ming-Hung Tsai wrote:
> The fix addresses more than crafted metadata. A lost write on disk
> drives can cause a stale block that was freed as a hint block,
> triggering this out-of-bounds access issue.

You're right, and the commit message claims the opposite. Its last
sentence should read:

  Arrays only ever read their own blocks, so a mismatch means the block is
  not one this array wrote: a crafted image, or a lost write leaving a
  stale block that was freed as a hint block.

Mikulas, that is a message-only change -- fold it in when applying if you
agree.

Bryam