daxfs: Bound the image-supplied counts that index kernel arrays - #18
Open
congwang-mk wants to merge 1 commit into
Open
daxfs: Bound the image-supplied counts that index kernel arrays#18congwang-mk wants to merge 1 commit into
congwang-mk wants to merge 1 commit into
Conversation
overlay_bucket_count, pcache_slot_count and inode_count come straight from the image, and each one bounds an array the kernel then indexes without re-checking: bucket_mask = bucket_count - 1, applied to every probe in overlay_lookup() and overlay_insert(), which read and CAS-write &ovl->buckets[probe] hash_mask = slot_count - 1, applied to every probe in the pcache &info->base_inodes[ino - 1], guarded only by ino <= base_inode_count daxfs_mem_ptr() validates the start offset alone, so a forged count makes those accesses run past the mapped region. The overlay and pcache headers are worse: bucket_offset, pool_offset, pool_size, slot_meta_offset and slot_data_offset were validated nowhere at all, not even under the validate mount option, and the header itself was dereferenced straight off an unchecked daxfs_mem_ptr() result. Check each of these where the value becomes a pointer or an array bound. A forged bucket_count of 0x10000000 in a 256 MB image is now refused with "overlay bucket/pool exceed the overlay region" instead of probing four gigabytes past the end of the mapping. The validate mount option is deliberately left alone. These are preconditions for memory safety rather than data-quality checks, so they belong on the default path, but the O(n) structural and per-inode scans in validate.c stay opt-in as before. Tested with a throwaway mkdaxfs that forges the superblock count. A non-power-of-two count is rejected at overlay init where the unfixed module logged "overlay initialized (3 buckets)" and carried on; an oversized count is rejected before any bucket is touched. The oversized case was not run against the unfixed module on purpose, since that path CAS-writes outside the mapping. Existing images are unaffected: the suite passes 20/20 across static, split and empty modes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Three counts come straight from the image, and each one bounds an array the kernel then indexes without re-checking:
overlay_bucket_countbucket_mask = count - 1, applied to every probe inoverlay_lookup()andoverlay_insert(), which read and CAS-write&ovl->buckets[probe]pcache_slot_counthash_mask = count - 1, applied to every pcache probeinode_count&info->base_inodes[ino - 1], guarded only byino <= base_inode_countdaxfs_mem_ptr()validates the start offset alone, so a forged count makes those accesses run past the mapped region.The overlay and pcache headers are worse.
bucket_offset,pool_offset,pool_size,slot_meta_offsetandslot_data_offsetwere validated nowhere at all — not even under thevalidatemount option, which only covers superblock-level fields. And in bothdaxfs_overlay_init()anddaxfs_pcache_init()the header was dereferenced straight off an uncheckeddaxfs_mem_ptr()result, so an out-of-range region offset is a NULL dereference before any of that.overlay_pool_ptr()bounds pool offsets againsthdr->pool_size, which is itself the untrusted value, so it validates nothing.This matters more for daxfs than for a local disk filesystem: the image lives in memory shared with other kernels or CXL hosts, so "the image is trusted" is a weaker assumption than usual.
Fix
Check each value where it becomes a pointer or an array bound:
daxfs_overlay_init()— header must be mapped before it is read;bucket_countnon-zero and a power of two (bucket_masksilently stops covering the array otherwise); bucket and pool spans inside both the overlay region and the mapping; bucket array must not overlap the pool. Validation moved ahead of thekzallocso the error paths do not need to free.daxfs_pcache_init()— same treatment for the header and the two slot arrays.daxfs_fill_super()— base inode table must be mapped for the declaredinode_count.The
validatemount option is deliberately unchanged. An earlier revision of this patch madedaxfs_validate_super()unconditional, which was the wrong call: it changes what the option means and risks rejecting older images that mount today. These checks are preconditions for memory safety rather than data-quality checks, so they belong on the default path, but the O(n) structural and per-inode scans invalidate.cstay opt-in exactly as before. Bothctx->validatecall sites are untouched.Testing
Built a throwaway
mkdaxfsthat forges the superblock's bucket count while laying the image out normally, simulating an image daxfs did not produce.That last line is the point: the unfixed module accepted the forged count and built the table. Its mount did fail, but only downstream, when the root inode lookup missed because the entry had been hashed with the real 65536-bucket mask and was looked up with mask 2. A symptom, not a check.
The oversized count was not run against the unfixed module on purpose. That path CAS-writes outside the mapping, and the test host is a shared development machine.
No regression:
tests/test_overlay.shpasses 20/20 across static, split and empty modes, so images produced by the realmkdaxfsare unaffected.Not covered here
overlay_lookup()still degrades to O(bucket_count) once the table fills, which is a separate issue from the review.