mbox series

[00/42,SRU,U] Fix can't find root device on VMD when secureboot enabled

Message ID 20200526125954.237403-1-vicamo.yang@canonical.com
Headers show
Series Fix can't find root device on VMD when secureboot enabled | expand

Message

You-Sheng Yang May 26, 2020, 12:59 p.m. UTC 
BugLink: https://bugs.launchpad.net/bugs/1876707

[Impact]

On platforms with NVMe attached to VMD controller, enable SecureBoot
would also force enable iommu:

  DMAR: Intel-IOMMU force enabled due to platform opt in

While devices behind the VMD controller, also a PCI bridge, maybe forced
to use a DMA domain by current Intel-IOMMU driver, this may break some
relationships between sub devices behind VMD controller and between the
VMD controller and its children devices, and finally caused undefined
system behavior.

On devices at hand, this results in kernel NULL dereference at
__intel_map_single called from nvme_reset_work and fails root device
lookup at boot.

  kernel: BUG: kernel NULL pointer dereference, address: 0000000000000018
  kernel: #PF: supervisor read access in kernel mode
  kernel: #PF: error_code(0x0000) - not-present page
  kernel: PGD 0 P4D 0
  kernel: Oops: 0000 [#2] SMP NOPTI
  kernel: CPU: 1 PID: 254 Comm: kworker/u8:4 Tainted: G D W
  5.7.0-050700rc3-generic #202004262131
  kernel: Hardware name: Dell Inc. Vostro 5402/, BIOS 0.1.2 04/13/2020
  kernel: Workqueue: nvme-reset-wq nvme_reset_work [nvme]
  kernel: RIP: 0010:__intel_map_single+0xa3/0x1a0
  ...

[Fix]

Patchset[1] currently landed in iommu/next beginning with commit
327d5b2fee91 ("iommu/vt-d: Allow 32bit devices to uses DMA domain")
gives the solution to this problem. However, it's based on a massive
subsystem rewrite in patchset[2], currently in iommu/next beginning with
commit 441ff2ff8327 ("Move default domain allocation to separate
function").

On v5.6, it also depends on yet a few more patch series landed in
v5.7-rc1 beginning with commit 098accf2da94 ("iommu: Use C99 flexible
array in fwspec") that rewrote private data access, changed struct
names, etc.

Yet a few additional patches included as fixes to above changes.

[1]: https://lore.kernel.org/linux-iommu/7928dd48-93da-62f0-b455-6e6b248d0fae@linux.intel.com/
[2]: https://lore.kernel.org/linux-iommu/20200429133712.31431-1-joro@8bytes.org/

[Test Case]

Test on platforms with VMD/NVMe and enable SecureBoot. System should
boot normally rather than into initramfs emergency shell.

[Regression Potential]

For unstable, all the patches are from iommu-next and will probably be
merged in next few -rc releases, so should be safe to place a LOW here.

For oem-5.6, the fixing patchset is depending on iommu group setup
refactoring that touched almost every architecture/platform uses iommu
although we would only care amd64 among them. Even with follow-up fixes
included, this is still a 60-patches change and deserves some more
attention. Medium.

Arnd Bergmann (1):
  iommu/renesas: Fix unused-function warning

Joerg Roedel (34):
  iommu: Move default domain allocation to separate function
  iommu/amd: Implement iommu_ops->def_domain_type call-back
  iommu/vt-d: Wire up iommu_ops->def_domain_type
  iommu/amd: Remove dma_mask check from check_device()
  iommu/amd: Return -ENODEV in add_device when device is not handled by
    IOMMU
  iommu: Add probe_device() and release_device() call-backs
  iommu: Move default domain allocation to iommu_probe_device()
  iommu: Keep a list of allocated groups in __iommu_probe_device()
  iommu: Move new probe_device path to separate function
  iommu: Split off default domain allocation from group assignment
  iommu: Move iommu_group_create_direct_mappings() out of
    iommu_group_add_device()
  iommu: Export bus_iommu_probe() and make is safe for re-probing
  iommu/amd: Remove dev_data->passthrough
  iommu/amd: Convert to probe/release_device() call-backs
  iommu/vt-d: Convert to probe/release_device() call-backs
  iommu/arm-smmu: Convert to probe/release_device() call-backs
  iommu/pamu: Convert to probe/release_device() call-backs
  iommu/s390: Convert to probe/release_device() call-backs
  iommu/virtio: Convert to probe/release_device() call-backs
  iommu/msm: Convert to probe/release_device() call-backs
  iommu/mediatek: Convert to probe/release_device() call-backs
  iommu/mediatek-v1 Convert to probe/release_device() call-backs
  iommu/qcom: Convert to probe/release_device() call-backs
  iommu/rockchip: Convert to probe/release_device() call-backs
  iommu/tegra: Convert to probe/release_device() call-backs
  iommu/renesas: Convert to probe/release_device() call-backs
  iommu/omap: Remove orphan_dev tracking
  iommu/omap: Convert to probe/release_device() call-backs
  iommu/exynos: Use first SYSMMU in controllers list for IOMMU core
  iommu/exynos: Convert to probe/release_device() call-backs
  iommu: Remove add_device()/remove_device() code-paths
  iommu: Move more initialization to __iommu_probe_device()
  iommu: Unexport iommu_group_get_for_dev()
  iommu: Don't call .probe_finalize() under group->mutex

Lu Baolu (3):
  iommu/vt-d: Allow 32bit devices to uses DMA domain
  iommu/vt-d: Allow PCI sub-hierarchy to use DMA domain
  iommu/vt-d: Apply per-device dma_ops

Qian Cai (1):
  iommu/amd: Fix variable "iommu" set but not used

Sai Praneeth Prakhya (2):
  iommu: Add def_domain_type() callback in iommu_ops
  iommu: Remove functions that support private domain

Tero Kristo via iommu (1):
  iommu/omap: Add check for iommu group when no IOMMU in use

 drivers/iommu/amd_iommu.c       | 100 +++----
 drivers/iommu/amd_iommu_types.h |   1 -
 drivers/iommu/arm-smmu-v3.c     |  38 +--
 drivers/iommu/arm-smmu.c        |  39 ++-
 drivers/iommu/exynos-iommu.c    |  24 +-
 drivers/iommu/fsl_pamu_domain.c |  22 +-
 drivers/iommu/intel-iommu.c     | 460 +++-----------------------------
 drivers/iommu/iommu.c           | 458 +++++++++++++++++++++----------
 drivers/iommu/ipmmu-vmsa.c      |  59 ++--
 drivers/iommu/msm_iommu.c       |  34 +--
 drivers/iommu/mtk_iommu.c       |  24 +-
 drivers/iommu/mtk_iommu_v1.c    |  50 ++--
 drivers/iommu/omap-iommu.c      | 102 ++-----
 drivers/iommu/qcom_iommu.c      |  24 +-
 drivers/iommu/rockchip-iommu.c  |  26 +-
 drivers/iommu/s390-iommu.c      |  22 +-
 drivers/iommu/tegra-gart.c      |  24 +-
 drivers/iommu/tegra-smmu.c      |  31 +--
 drivers/iommu/virtio-iommu.c    |  41 +--
 include/linux/iommu.h           |  33 ++-
 20 files changed, 563 insertions(+), 1049 deletions(-)

Comments

Seth Forshee June 1, 2020, 3:56 p.m. UTC | #1 
On Tue, May 26, 2020 at 08:59:12PM +0800, You-Sheng Yang wrote:
> BugLink: https://bugs.launchpad.net/bugs/1876707
> 
> [Impact]
> 
> On platforms with NVMe attached to VMD controller, enable SecureBoot
> would also force enable iommu:
> 
>   DMAR: Intel-IOMMU force enabled due to platform opt in
> 
> While devices behind the VMD controller, also a PCI bridge, maybe forced
> to use a DMA domain by current Intel-IOMMU driver, this may break some
> relationships between sub devices behind VMD controller and between the
> VMD controller and its children devices, and finally caused undefined
> system behavior.
> 
> On devices at hand, this results in kernel NULL dereference at
> __intel_map_single called from nvme_reset_work and fails root device
> lookup at boot.
> 
>   kernel: BUG: kernel NULL pointer dereference, address: 0000000000000018
>   kernel: #PF: supervisor read access in kernel mode
>   kernel: #PF: error_code(0x0000) - not-present page
>   kernel: PGD 0 P4D 0
>   kernel: Oops: 0000 [#2] SMP NOPTI
>   kernel: CPU: 1 PID: 254 Comm: kworker/u8:4 Tainted: G D W
>   5.7.0-050700rc3-generic #202004262131
>   kernel: Hardware name: Dell Inc. Vostro 5402/, BIOS 0.1.2 04/13/2020
>   kernel: Workqueue: nvme-reset-wq nvme_reset_work [nvme]
>   kernel: RIP: 0010:__intel_map_single+0xa3/0x1a0
>   ...
> 
> [Fix]
> 
> Patchset[1] currently landed in iommu/next beginning with commit
> 327d5b2fee91 ("iommu/vt-d: Allow 32bit devices to uses DMA domain")
> gives the solution to this problem. However, it's based on a massive
> subsystem rewrite in patchset[2], currently in iommu/next beginning with
> commit 441ff2ff8327 ("Move default domain allocation to separate
> function").
> 
> On v5.6, it also depends on yet a few more patch series landed in
> v5.7-rc1 beginning with commit 098accf2da94 ("iommu: Use C99 flexible
> array in fwspec") that rewrote private data access, changed struct
> names, etc.
> 
> Yet a few additional patches included as fixes to above changes.
> 
> [1]: https://lore.kernel.org/linux-iommu/7928dd48-93da-62f0-b455-6e6b248d0fae@linux.intel.com/
> [2]: https://lore.kernel.org/linux-iommu/20200429133712.31431-1-joro@8bytes.org/
> 
> [Test Case]
> 
> Test on platforms with VMD/NVMe and enable SecureBoot. System should
> boot normally rather than into initramfs emergency shell.
> 
> [Regression Potential]
> 
> For unstable, all the patches are from iommu-next and will probably be
> merged in next few -rc releases, so should be safe to place a LOW here.
> 
> For oem-5.6, the fixing patchset is depending on iommu group setup
> refactoring that touched almost every architecture/platform uses iommu
> although we would only care amd64 among them. Even with follow-up fixes
> included, this is still a 60-patches change and deserves some more
> attention. Medium.

All of these patches are also in linux-next. In that case they should
not be marked as SAUCE, and the "cherry picked from" lines should say
linux-next rather than iommu/next. And generally, whenever referring to
a tree other than upstream or linux-next, you should use the full git
URI rather than some shorthand like iommu/next as it cannot be assumed
that everyone knows where to look for that tree.

Thanks,
Seth