fix(core): split PRP chunks by DMA address, not user page offset - #23
Closed
sungjoo-XCENA wants to merge 1 commit into
Closed
fix(core): split PRP chunks by DMA address, not user page offset#23sungjoo-XCENA wants to merge 1 commit into
sungjoo-XCENA wants to merge 1 commit into
Conversation
mx_prp_first_chunk_len() derived the first chunk length from the CPU-side page offset (sg->offset), while the chunk itself is placed at sg_dma_address(). The device computes the first chunk length from the DMA address it receives, so whenever the mapping does not preserve the buffer's low address bits -- e.g. SWIOTLB bounce buffering -- every PRP entry after the first pointed at the wrong address. Transfers needing more than one PRP entry then silently corrupted data (shifted by the alignment delta) while the ioctl still returned success. Reproduced in a QEMU guest (no vIOMMU, SWIOTLB active): a 2576-byte DeviceInfo read at user page offset 1 was bounced to a 1 KiB-aligned DMA address; the driver emitted the desc list [base, base+1023, base+2047] instead of [base, base+1024, base+2048], shifting all data past byte 1023 by one. Direct mapping and IOMMU setups preserve the intra-page offset, which is why the bug stayed hidden on real hardware. Also branch create_mx_command_sg() (v2) on the DMA-side descriptor count instead of a host page count computed from the user virtual address, which could disagree with the DMA layout the same way. The SINGLE_DMA_SIZE == PAGE_SIZE static_assert only guarded that host-page branching and goes away with it.
Author
|
Wrong repo — resubmitted to the dev repo: https://github.com/xcena-dev/mxdriver_dev/pull/86 |
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
mx_prp_first_chunk_len()computed the first PRP chunk length from the CPU-side page offset (sg->offset), but the chunk itself is placed atsg_dma_address(). The device derives the first chunk length from the DMA address it receives, so whenever the mapping does not preserve the buffer's low address bits, every PRP entry after the first points at the wrong address. Any transfer needing more than one PRP entry then silently corrupts data — shifted by the alignment delta — while the ioctl still returns 0.The mismatch occurs when
user_offset % dma_size != dma_addr % dma_size. Direct mapping and IOMMU translation preserve the intra-page offset, so the bug stays hidden there; SWIOTLB bounce buffering does not (nodma_min_align_maskis set), so it corrupts. SWIOTLB engages with no IOMMU + buffers outside the device's reach, in confidential VMs (SEV/TDX), or withswiotlb=force.Reproduction (QEMU guest, no vIOMMU, SWIOTLB active)
2576-byte DeviceInfo read on the v1 queue (
dma_size=1024), same run, only buffer offset differs:0x7cfe9000(aligned)0x7e073800(slot-aligned)0x7e073800, 0x7e073bff, 0x7e073fffThe device writes 1024 bytes from
0x7e073800(address is 1 KiB-aligned), then jumps to the second entry0x7e073bff, overwriting the byte it just wrote: all data past byte 1023 shifts down by one (hmboxRqOffsetreads back0x10instead of0x1000; payload FNV-1a hash differs from the aligned run). ioctl returns 0 in both cases. A read-size matrix confirms the boundary: sizes ≤ 1024 pass at any offset, sizes ≥ 1025 corrupt at offsets 1/511/1023.Fix
mx_prp_first_chunk_len(): derive the split fromsg_dma_address(sg) + intra_offinstead ofsg->offset. This fixes all callers (v1 single/multi decision, v2 two-entry path, desc-list walk and count).create_mx_command_sg()(v2): branch on the DMA-side descriptor count (mx_get_total_desc_count()) instead of a host page count computed from the user virtual address — same failure mode. TheSINGLE_DMA_SIZE == PAGE_SIZEstatic_assert only guarded that host-page branching and goes away with it.transfer.cstill uses the user offset for page pinning, which is CPU-side and correct.With the fix the same transfer emits
0x7e073800, 0x7e073c00, 0x7e074000.Testing
6.8.0-124-generic.DMA-FQ) are unaffected before and after — behavior only changes where SWIOTLB previously corrupted.