Re: [PATCH] iommu: Allocate dev_iommu before accessing priv data

Wed, 02 Sep 2020 23:53:39 -0700 

Hi Robin,

On 9/2/20 7:31 PM, Robin Murphy wrote:

On 2020-09-02 06:32, Torsten Hilbrich wrote:

After updating from v5.8 to v5.9-rc2 I noticed some problems when
booting a system with kernel cmdline "intel_iommu=on,igfx_off".

The following stacktrace was produced:
<6>[    0.000000] Command line: BOOT_IMAGE=/isolinux/bzImage console=tty1 intel_iommu=on,igfx_off 
...
<6>[    3.341682] DMAR: Host address width 39
<6>[    3.341684] DMAR: DRHD base: 0x000000fed90000 flags: 0x0
<6>[    3.341702] DMAR: dmar0: reg_base_addr fed90000 ver 1:0 cap 1c0000c40660462 ecap 19e2ff0505e 
<6>[    3.341705] DMAR: DRHD base: 0x000000fed91000 flags: 0x1
<6>[    3.341711] DMAR: dmar1: reg_base_addr fed91000 ver 1:0 cap d2008c40660462 ecap f050da 
<6>[    3.341713] DMAR: RMRR base: 0x0000009aa9f000 end: 0x0000009aabefff
<6>[    3.341716] DMAR: RMRR base: 0x0000009d000000 end: 0x0000009f7fffff
<6>[    3.341726] DMAR: No ATSR found
<1>[    3.341772] BUG: kernel NULL pointer dereference, address: 0000000000000038 
<1>[    3.341774] #PF: supervisor write access in kernel mode
<1>[    3.341776] #PF: error_code(0x0002) - not-present page
<6>[    3.341777] PGD 0 P4D 0
<4>[    3.341780] Oops: 0002 [#1] SMP PTI
<4>[    3.341783] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.9.0-devel+ #2 <4>[    3.341785] Hardware name: LENOVO 20HGS0TW00/20HGS0TW00, BIOS N1WET46S (1.25s ) 03/30/2018 
<4>[    3.341790] RIP: 0010:intel_iommu_init+0xed0/0x1136
<4>[    3.341792] Code: fe e9 61 02 00 00 bb f4 ff ff ff e9 57 02 00 00 48 63 d1 48 c1 e2 04 48 03 50 20 48 8b 12 48 85 d2 74 0b 48 8b 92 d0 02 00 00 <48> 89 7a 38 ff c1 e9 15 f5 ff ff 48 c7 c7 60 99 ac a7 49 c7 c7 a0 
<4>[    3.341796] RSP: 0000:ffff96d180073dd0 EFLAGS: 00010282
<4>[    3.341798] RAX: ffff8c91037a7d20 RBX: 0000000000000000 RCX: 0000000000000000 <4>[    3.341800] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffffffffff <4>[    3.341802] RBP: ffff96d180073e90 R08: 0000000000000001 R09: ffff8c91039fe3c0 <4>[    3.341804] R10: 0000000000000226 R11: 0000000000000226 R12: 000000000000000b <4>[    3.341806] R13: ffff8c910367c650 R14: ffffffffa8426d60 R15: 0000000000000000 <4>[    3.341808] FS:  0000000000000000(0000) GS:ffff8c9107480000(0000) knlGS:0000000000000000 
<4>[    3.341810] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[    3.341812] CR2: 0000000000000038 CR3: 00000004b100a001 CR4: 00000000003706e0 
<4>[    3.341814] Call Trace:
<4>[    3.341820]  ? _raw_spin_unlock_irqrestore+0x1f/0x30
<4>[    3.341824]  ? call_rcu+0x10e/0x320
<4>[    3.341828]  ? trace_hardirqs_on+0x2c/0xd0
<4>[    3.341831]  ? rdinit_setup+0x2c/0x2c
<4>[    3.341834]  ? e820__memblock_setup+0x8b/0x8b
<4>[    3.341836]  pci_iommu_init+0x16/0x3f
<4>[    3.341839]  do_one_initcall+0x46/0x1e4
<4>[    3.341842]  kernel_init_freeable+0x169/0x1b2
<4>[    3.341845]  ? rest_init+0x9f/0x9f
<4>[    3.341847]  kernel_init+0xa/0x101
<4>[    3.341849]  ret_from_fork+0x22/0x30
<4>[    3.341851] Modules linked in:
<4>[    3.341854] CR2: 0000000000000038
<4>[    3.341860] ---[ end trace 3653722a6f936f18 ]---
I could track the problem down to the dev_iommu_priv_set call in the function init_no_remapping_devices in the path where !dmar_map_gfx. It turned out that 
the dev->iommu entry is NULL at this time.

Lu Baolu <[email protected]> suggested for dev_iommu_priv_set
to automatically allocate the iommu entry by using the function
dev_iommu_get to retrieve that pointer. This function allocates the
entry if needed.

Fixes: 01b9d4e21148 ("iommu/vt-d: Use dev_iommu_priv_get/set()")
Signed-off-by: Torsten Hilbrich <[email protected]>
Tested-by: Torsten Hilbrich <[email protected]>
Link: https://lists.linuxfoundation.org/pipermail/iommu/2020-August/048098.html 
---
  drivers/iommu/iommu.c | 22 ++++++++++++++++++++++
  include/linux/iommu.h | 11 ++---------
  2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 609bd25bf154..3edca2a31296 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2849,3 +2849,25 @@ int iommu_sva_get_pasid(struct iommu_sva *handle)
      return ops->sva_get_pasid(handle);
  }
  EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
+
+void *dev_iommu_priv_get(struct device *dev)
+{
+       struct dev_iommu *param = dev_iommu_get(dev);
+
+       if (WARN_ON(!param))
+               return ERR_PTR(-ENOMEM);
+
+        return param->priv;
+}
+EXPORT_SYMBOL_GPL(dev_iommu_priv_get);
Hmm, I'm not convinced by this - it looks it would only paper over real driver bugs. If the driver's calling dev_iommu_priv_get(), it presumably wants to actually *do* something with its private data - if it somehow manages to make that call before it's processed ->probe_device(), it can't possibly get *meaningful* data, so even if we stop that call from crashing how can it result in correct behaviour?
And if the device isn't managed by that IOMMU driver, then it shouldn't be calling dev_iommu_priv_get() blindly in the first place (and allocating redundant structures would just be a waste). 

Fair enough. The dev_iommu_priv_g(s)et() apis shouldn't be abused.




+void dev_iommu_priv_set(struct device *dev, void *priv)
+{
+       struct dev_iommu *param = dev_iommu_get(dev);
+
+       if (WARN_ON(!param))
+               return;
+
+        param->priv = priv;
+}
+EXPORT_SYMBOL_GPL(dev_iommu_priv_set);
In this direction it's at least not completely illogical, but it's still indicative of a driver operating very much outside the expected API flow. If a driver has special knowledge of devices it manages before it's seen ->probe_device() for them then fair enough, but it should probably be that driver's responsibility to manage any "out of order" usage of its private data as a special case, rather than pretending that this is expected common behaviour. Again, for most drivers it's more likely to just mask bugs than be genuinely useful. 

Agreed. I will post a fix by addressing this bug in the VT-d driver.

Best regards,
baolu



Robin.


diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index fee209efb756..e3e725cf64b3 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -609,15 +609,8 @@ static inline void dev_iommu_fwspec_set(struct device *dev, 
      dev->iommu->fwspec = fwspec;
  }
-static inline void *dev_iommu_priv_get(struct device *dev)
-{
-    return dev->iommu->priv;
-}
-
-static inline void dev_iommu_priv_set(struct device *dev, void *priv)
-{
-    dev->iommu->priv = priv;
-}
+void *dev_iommu_priv_get(struct device *dev);
+void dev_iommu_priv_set(struct device *dev, void *priv);
  int iommu_probe_device(struct device *dev);
  void iommu_release_device(struct device *dev);


_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Reply via email to