Re: [PATCH] dm array: validate array block headers on read

On Tue, Jul 28, 2026 at 8:16=E2=80=AFAM Bryam Vargas via B4 Relay
<[email protected]> wrote:
>
> From: Bryam Vargas <[email protected]>
>
> ---
> Found by reading array_block_check() next to node_check(), its opposite n=
umber for btree
> nodes in the same directory.  node_check() already bounded max_entries an=
d nr_entries when
> dm-array was added in 2013; the array validator never picked it up.
>
> Reproducer.  Create a cache on a zeroed metadata device so dm-cache forma=
ts it, tear that
> down, then set nr_entries in the first mapping array block to 0xFFFFFFFF =
and recompute the
> block checksum.  dm_bm_checksum() is crc32c(~0, data, len) ^ CSUM_XOR, so=
 the checksum costs
> nothing to forge -- it is a corruption check, not a tag.  The crafted ima=
ge differs from the
> one the kernel itself wrote by four bytes plus that checksum.
>
> A 4096-byte block with a 24-byte header and 8-byte values holds 509 entri=
es, so index 509 is
> the first one outside it.
>
> A/B on v7.2-rc4-610 with KASAN and kasan.fault=3Dreport, this patch built=
 as posted:
>
>   pristine image, unpatched:  activates, walks all 2048 cache blocks, dme=
sg empty
>   crafted image,  unpatched:  reads past the block, below
>   crafted image,  patched:    "array_block_check failed: too many entries=
", -EILSEQ
>   pristine image, patched:    activates, dmesg empty
>
>     BUG: KASAN: slab-use-after-free in dm_cache_load_mappings+0x9b1/0xb40=
 [dm_cache]
>     Read of size 8 at addr ffff888107fc8000 by task dmsetup/1667
>      dm_cache_load_mappings+0x9b1/0xb40 [dm_cache]
>      cache_preresume+0x4cd/0xd70 [dm_cache]
>      dm_table_resume_targets+0xcd/0x2e0 [dm_mod]
>      ctl_ioctl+0x512/0xa90 [dm_mod]
>      __x64_sys_ioctl+0x134/0x1c0
>     The buggy address is located 0 bytes inside of
>      freed 32-byte region [ffff888107fc8000, ffff888107fc8020)
>
> The block base was ffff888107fc7000, so that address is base + 0x1000: by=
te 4096, index 509,
> the first entry outside.
>
> Worth flagging about that KASAN line: it corroborates, it does not reprod=
uce on demand.  Two
> earlier runs of the same arm produced no report at all, even though the o=
verread happened
> both times and dm-cache said so by refusing a nonsense mapping at cache b=
lock 509.  dm-bufio
> serves this buffer from __get_free_pages(), the page allocator has no red=
zones, and byte 4096
> is the first byte of the next page, so KASAN can only speak when that pag=
e happens to be
> poisoned.  The first line is the deterministic one.
>
> The check is not conservative.  calc_max_entries() gives 509 for 8-byte v=
alues and 1018 for
> 4-byte ones, and 24 + 8*509 and 24 + 4*1018 are both exactly 4096, so met=
adata dm-array wrote
> sits right at the limit and still passes.

Thank you for the patch. I agree with validating these header fields,
and I have a slightly different proposal: compare max_entries against
"calc_max_entries(value_size, size_of_block)" in array_block_check().
This prevents callers from hitting BUG_ON in fill_ablock() or
trim_ablock() when a max_entries is smaller than the expected
calc_max_entries().

Adding the equality check has no compatibility concerns. dm-array's
calc_max_entries() is unchanged since it was introduced in 2013. Also,
the max_entries is used by dm_array_get_value() to address entries;
any further change to the max_entries formula would break addressing
on existing metadata.

Checking "!value_size" becomes necessary to avoid divide-by-zero in
calc_max_entries(). I'd prefer splitting the value_size check from
max_entries with a different error message for diagnosability. A
combined form "if (!value_size || max_entries !=3D ...)" also works, but
the shared error message needs some tweaks, maybe printing both
value_size and max_entries, not just "max_entries too large".

> There is a second hole I am not fixing here, since it wants its own repro=
ducer and this one is
> headed for stable.  A validator can only check a header against itself, s=
o a block with
> value_size 4 and max_entries 1018 is internally consistent and gets throu=
gh, but element_at()
> indexes with the caller's value size rather than the stored one -- and dm=
-cache keeps arrays of
> both, mappings at 8 and hints at 4.  Read an array block of one through t=
he other and the walk
> runs to byte 8160 of a 4096-byte block.  Closing that means comparing val=
ue_size against
> info->value_type.size in get_ablock() and in __shadow_ablock(), the two p=
laces that hold both
> the block and the caller.
>
> dm-era reaches the same accessor directly and dm-clone reaches it through=
 dm-bitset, which is
> an array underneath; dm-cache is only the shortest path from a crafted im=
age to an observable
> read.  All of them need CAP_SYS_ADMIN to load the table, which is why I'm=
 sending this as
> hardening rather than as a security report.
>
> The value_size patch follows once I have a reproducer for it.

That kind of contextual error is a concern. It could be reproduced by
a crafted metadata with swapped root nodes for mappings and hints in
superblock (with updated checksum). During device activation and
__load_mappings(), dm_array_cursor_next() iterates entries according
to the cached array block's nr_entries, while
dm_array_cursor_get_value() reads the entries in terms of
info->value_size, triggerring out-of-bounds access,

cache_check and era_check v1.0+ already detect value_size
inconsistencies, and older thin-provisioning-tools can detect the
error by checking illegal mappings and flags, that helps mitigate the
risk offline. An in-kernel follow-up patch is still worthwhile for
online validation.


> ---