Re: [PATCH v2 0/9] iommu/vt-d: Improve PASID id and table management

[Lu Baolu <baolu.lu@linux.intel.com>]

Hi,

On 05/16/2018 04:56 PM, Tian, Kevin wrote:
>> From: Lu Baolu [mailto:baolu.lu@linux.intel.com]
>> Sent: Wednesday, May 16, 2018 4:01 PM
>>
>> Hi Joerg,
>>
>> Thank you for looking at my patches.
>>
>> On 05/15/2018 10:11 PM, Joerg Roedel wrote:
>>> On Fri, May 04, 2018 at 09:41:15AM +0800, Lu Baolu wrote:
>>>> PATCH 4~9 implement per domain PASID table. Current per IOMMU
>>>> PASID table implementation is insecure in the cases where
>>>> multiple devices under one single IOMMU unit support PASID
>>>> feature. With per domain PASID table, we can achieve finer
>>>> protection and isolation granularity.
>>> Hold on, we hat discussions in the past about doing a system-wide pasid
>>> space, so that every mm_struct with devices attached gets the same pasid
>>> across all devices it is talking to. Reason was that some devices (will)
>>> require this to work correctly. This goes into the opposite direction,
>>> so I am a bit confused here. Please explain, is this not longer
>>> necessary?
>> You are right. System-wide pasid space is necessary, hence PATCH
>> 1~3 implement it. But PATCH 4~9 don't go into the opposite direction,
>> it's designed to address another potential issue.
> one thing you may want to highlight is, even with PATCH 4-9 it's
> still doing system-wide PASID space allocation. Just PASID table
> itself is kept per-device for isolation purpose as you described
> below, i.e. each device can access only those PASIDs which are
> allocated to itself while the allocation happens system-wide...

Yes, exactly.

Best regards,
Lu Baolu

>
>> With system-wide pasid space, we can use a system-wide pasid table,
>> or just keep what we have now(per iommu unit pasid table). Both
>> system-wide and per iommu unitpasid table mean that two devices
>> might share a single pasid table. That will result in an issue.
>>
>> For an example, device A is assigned to access the memory space of
>> process A, and device B is assigned to access the memory space of
>> process B. The dma remapping infrastructure looks like:
>>
>>                                                      .------------------.
>>                              .----------------.      |                  |
>>                              |                |      |                  |
>>                              .----------------.      | Paging structure |
>>                              |     PASID X    |--|   |  for Process A   |
>>                              .----------------.  |   |                  |
>>                              |                |  --->'------------------'
>>      .----------------.      .----------------.
>>      |                |      |     PASID Y    |--|
>>      .----------------.      .----------------.  |
>>      | Dev_A context  |---|  |                |  |   .------------------.
>>      '----------------'   |  .----------------.  |   |                  |
>>      |                |   |  |                |  |   |                  |
>>      '----------------'   |  .----------------.  |   | Paging structure |
>>      | Dev_B context  |   -->|                |  |   |  for Process B   |
>>      '----------------'----->'----------------'  |   |                  |
>>      |                |         system-wide      v-->'------------------'
>>      .----------------.         pasid table
>>      |                |
>>      '----------------'
>>         Intel iommu
>>        context table
>>
>>
>> Since dev_A and dev_B share a pasid table, the side effect is that a flawed
>> dev_A might access the memory space of process B (with pasid y). Vice
>> versa,
>> a flawed dev_B might access memory space of process A (with pasid x).
>>
>> What PATCH 4~9 do is to remove such possibility by assigning a pasid table
>> for each pci device. Hence, the remapping infrastructure looks like:
>>
>>
>>                                                     .------------------.
>>                                                     |                  |
>>                             .----------------.      |                  |
>>                             |                |      | Paging structure |
>>                             .----------------.      |  for Process A   |
>>                             |     PASID X    |      |                  |
>>                             .----------------.----->'------------------'
>>                             |                |
>>                             .----------------.
>>                             |                |
>>                             .----------------.
>>                             |                |
>>                             .----------------.
>>    .----------------.       |                |
>>    |                |       .----------------.
>>    .----------------.       |                |
>>    | Dev_A context  |------>'----------------'
>>    '----------------'          pasid table
>>    |                |          for Dev_A
>>    '----------------'
>>    | Dev_B context  |-->
>>    '----------------'  |    .----------------.
>>    |                |  |    |                |      .------------------.
>>    .----------------.  |    .----------------.      |                  |
>>    |                |  |    |                |      |                  |
>>    '----------------'  |    .----------------.      | Paging structure |
>>        Intel iommu     |    |                |      |  for Process B   |
>>       context table    |    .----------------.      |                  |
>>                        |    |     PASID Y    |----->'------------------'
>>                        |    .----------------.
>>                        |    |                |
>>                        |    .----------------.
>>                        |    |                |
>>                        |    .----------------.
>>                        v--->|                |
>>                             '----------------'
>>                                pasid table
>>                                for Dev_B
>>
>>
>> With this, dev_A has no means to access memory of process B and vice
>> versa.
>>
>> Best regards,
>> Lu Baolu