Messages in this thread | | | Date | Thu, 5 Mar 2020 17:14:31 -0800 | From | Nicolin Chen <> | Subject | Possible bugs in iommu_map_sg()/iommu_map_dma_sg() |
| |
Hi all,
I recently ran a 4GB+ allocation test case with my downstream older-version kernel, and found two possible bugs. I then saw the mainline code, yet don't find them getting fixed.
However, I am not 100% sure that they are real practical bugs because I later figured out that my use case was not entirely correct. So I'd like to get some advice first, before sending any patch.
First problem is accumulating the pad_len in iommu_map_dma_sg. My use case was to map a size of 4GB+ sg while the device did not set its segmentation boundary -- leaving it to the default 32-bit mask.
00 of 14: s_length 90000, s->length 90000, iova_len 0 01 of 14: s_length 100000, s->length 100000, iova_len 90000 02 of 14: s_length 100000, s->length 100000, iova_len 190000 03 of 14: s_length 200000, s->length 200000, iova_len 290000 04 of 14: s_length 200000, s->length 200000, iova_len 490000 05 of 14: s_length 39c00000, s->length 39c00000, iova_len 690000 06 of 14: s_length 400000, s->length 400000, iova_len 3a290000 07 of 14: s_length 400000, s->length 400000, iova_len 3a690000 08 of 14: s_length 400000, s->length 400000, iova_len 3aa90000 09 of 14: s_length 400000, s->length 400000, iova_len 3ae90000 10 of 14: s_length 400000, s->length 400000, iova_len 3b290000 11 of 14: s_length 400000, s->length 400000, iova_len 3b690000 12 of 14: s_length fffff000, s->length fffff000, iova_len 3ba90000 12 of 14: prev->length 400000 + pad_len c4570000 = c4970000 13 of 14: s_length 1df41000, s->length 1df41000, iova_len 1fffff000 13 of 14: prev->length fffff000 + pad_len 1000 = 100000000
So, the problem here is adding the last pad_len 0x1000 to the prev->length 0xfffff000, and writing the result back: 880 if (pad_len && pad_len < s_length - 1) { 881 prev->length += pad_len;
This 0x100000000 overflows that "unsigned int" prev->length.
Second problem is in the iommu_map_sg function. When it maps iova to phys for each list, it uses previously padded length instead of the actual s->dma_length, which means it possibly maps some of the iova space to a physical address space that is out of the allocated region. For a large value of pad_len 0xc4570000, it might end up map iova to somewhere invalid: iova [0xc3b690000, 0xd00000000] ==> pa [0x0000000262800000, 0x0000000327170000] // size 0xc4970000, dma_size 0x400000
This 0x327170000 is out of actual physical address space for my platform. And even for the small 0x1000 pad_len, it still maps out of the allocated region.
For my use case, I made it work by setting the segmentation boundary to a larger size, which shouldn't be wrong because I need a contiguous iova space with no paddings in-between.
Yet, since the code is designed to take care of a "mask size < IOVA size" case, I feel that we probably need to fix these two issues.
For problem 1, should we change the type of length to size_t?
For problem 2, should we map each iova<=>pa using dma_length?
Thanks Nicolin
| |