Possible bugs in iommu_map_sg()/iommu_map_dma_sg()

[Nicolin Chen <nicoleotsuka@gmail.com>]

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