On Tue, Jul 28, 2026 at 8:16 AM 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 > number for btree > nodes in the same directory. node_check() already bounded max_entries and > 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 formats > 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 image > 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 entries, > so index 509 is > the first one outside it. > > A/B on v7.2-rc4-610 with KASAN and kasan.fault=report, this patch built as > posted: > > pristine image, unpatched: activates, walks all 2048 cache blocks, dmesg > 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: byte > 4096, index 509, > the first entry outside. > > Worth flagging about that KASAN line: it corroborates, it does not reproduce > on demand. Two > earlier runs of the same arm produced no report at all, even though the > overread happened > both times and dm-cache said so by refusing a nonsense mapping at cache block > 509. dm-bufio > serves this buffer from __get_free_pages(), the page allocator has no > redzones, and byte 4096 > is the first byte of the next page, so KASAN can only speak when that page > happens to be > poisoned. The first line is the deterministic one. > > The check is not conservative. calc_max_entries() gives 509 for 8-byte > values and 1018 for > 4-byte ones, and 24 + 8*509 and 24 + 4*1018 are both exactly 4096, so > metadata 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 != ...)" 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 > reproducer and this one is > headed for stable. A validator can only check a header against itself, so a > block with > value_size 4 and max_entries 1018 is internally consistent and gets through, > 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 the > other and the walk > runs to byte 8160 of a 4096-byte block. Closing that means comparing > value_size against > info->value_type.size in get_ablock() and in __shadow_ablock(), the two > places 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 image > 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. > ---
