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

[Tian, Kevin]

> From: Lu Baolu [mailto:baolu.lu@xxxxxxxxxxxxxxx]
> 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...

> 
> 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