linux-kernel - Possible bugs in iommu_map_sg()/iommu_map_dma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20200306011430.GA17529@Asurada-Nvidia.nvidia.com>
Date:   Thu, 5 Mar 2020 17:14:31 -0800
From:   Nicolin Chen <nicoleotsuka@...il.com>
To:     robin.murphy@....com, jroedel@...e.de
Cc:     iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org
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