All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Tian, Kevin" <kevin.tian@intel.com>
To: Lu Baolu <baolu.lu@linux.intel.com>, Joerg Roedel <joro@8bytes.org>
Cc: David Woodhouse <dwmw2@infradead.org>,
	"Raj, Ashok" <ashok.raj@intel.com>,
	"Kumar, Sanjay K" <sanjay.k.kumar@intel.com>,
	"Pan, Jacob jun" <jacob.jun.pan@intel.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>,
	"Sun, Yi Y" <yi.y.sun@intel.com>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2 0/9] iommu/vt-d: Improve PASID id and table management
Date: Wed, 16 May 2018 08:56:30 +0000	[thread overview]
Message-ID: <AADFC41AFE54684AB9EE6CBC0274A5D19116ADF6@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <5AFBE53C.2070604@linux.intel.com>

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

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

WARNING: multiple messages have this Message-ID (diff)
From: "Tian, Kevin" <kevin.tian-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
To: Lu Baolu <baolu.lu-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>,
	Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Cc: "Raj, Ashok" <ashok.raj-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
	"Kumar,
	Sanjay K"
	<sanjay.k.kumar-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
	"iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org"
	<iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>,
	"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"Sun, Yi Y" <yi.y.sun-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
	"Pan,
	Jacob jun"
	<jacob.jun.pan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
	David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Subject: RE: [PATCH v2 0/9] iommu/vt-d: Improve PASID id and table management
Date: Wed, 16 May 2018 08:56:30 +0000	[thread overview]
Message-ID: <AADFC41AFE54684AB9EE6CBC0274A5D19116ADF6@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <5AFBE53C.2070604-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

> From: Lu Baolu [mailto:baolu.lu-VuQAYsv1563Yd54FQh9/CA@public.gmane.org]
> 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

  reply	other threads:[~2018-05-16  8:56 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-04  1:41 [PATCH v2 0/9] iommu/vt-d: Improve PASID id and table management Lu Baolu
2018-05-04  1:41 ` Lu Baolu
2018-05-04  1:41 ` [PATCH v2 1/9] iommu/vt-d: Global PASID name space Lu Baolu
2018-05-04  1:41   ` Lu Baolu
2018-05-04  1:41 ` [PATCH v2 2/9] iommu/vt-d: Decouple idr bond pointer from svm Lu Baolu
2018-05-04  1:41   ` Lu Baolu
2018-05-04  1:41 ` [PATCH v2 3/9] iommu/vt-d: Use global PASID for SVM usage Lu Baolu
2018-05-04  1:41 ` [PATCH v2 4/9] iommu/vt-d: Move device_domain_info to header Lu Baolu
2018-05-04  1:41   ` Lu Baolu
2018-05-04  1:41 ` [PATCH v2 5/9] iommu/vt-d: Per domain pasid table interfaces Lu Baolu
2018-05-04  1:41   ` Lu Baolu
2018-05-04  1:41 ` [PATCH v2 6/9] iommu/vt-d: Allocate and free pasid table Lu Baolu
2018-05-04  1:41 ` [PATCH v2 7/9] iommu/vt-d: Calculate PTS value Lu Baolu
2018-05-04  1:41   ` Lu Baolu
2018-05-04  1:41 ` [PATCH v2 8/9] iommu/vt-d: Use per-domain pasid table Lu Baolu
2018-05-04  1:41 ` [PATCH v2 9/9] iommu/vt-d: Clean up PASID talbe management for SVM Lu Baolu
2018-05-15 14:11 ` [PATCH v2 0/9] iommu/vt-d: Improve PASID id and table management Joerg Roedel
2018-05-15 14:11   ` Joerg Roedel
2018-05-16  8:01   ` Lu Baolu
2018-05-16  8:01     ` Lu Baolu
2018-05-16  8:56     ` Tian, Kevin [this message]
2018-05-16  8:56       ` Tian, Kevin
2018-05-17  1:13       ` Lu Baolu
2018-05-17  1:13         ` Lu Baolu
2018-05-29 11:56     ` Joerg Roedel
2018-05-30  0:56       ` Lu Baolu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AADFC41AFE54684AB9EE6CBC0274A5D19116ADF6@SHSMSX101.ccr.corp.intel.com \
    --to=kevin.tian@intel.com \
    --cc=ashok.raj@intel.com \
    --cc=baolu.lu@linux.intel.com \
    --cc=dwmw2@infradead.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jacob.jun.pan@intel.com \
    --cc=joro@8bytes.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sanjay.k.kumar@intel.com \
    --cc=yi.l.liu@intel.com \
    --cc=yi.y.sun@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.