Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Incomplete Refactoring breaking Device Attachment for Shared IOMMUs. Pre-existing issues: - [High] Out-of-bounds Read / Undefined Behavior due to `list_first_entry()` on potentially empty `ctx_list`. - [High] Lockless Access / Race Condition during TLB flushes iterating over `ctx_list`. -- --- Patch [1]: [PATCH 1/3] iommu/msm: track a context master per device and IOMMU --- commit c8d40e4c746f720804d5ad115bf7e9dd560b624f Author: Dmitry Baryshkov <[email protected]> iommu/msm: track a context master per device and IOMMU insert_iommu_master() decided whether to allocate a struct msm_iommu_ctx_dev by testing whether the IOMMU's ctx_list was empty, caching the result in dev_iommu_priv, then unconditionally dereferenced the master. > diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c > index 4ef1e8c8244b0..aab87ccbcada9 100644 > --- a/drivers/iommu/msm_iommu.c > +++ b/drivers/iommu/msm_iommu.c > @@ -601,24 +601,27 @@ static int insert_iommu_master(struct device *dev, > struct msm_iommu_dev **iommu, > const struct of_phandle_args *spec) > { [ ... ] > - if (list_empty(&(*iommu)->ctx_list)) { > - master = kzalloc_obj(*master, GFP_ATOMIC); > - if (!master) { > - dev_err(dev, "Failed to allocate iommu_master\n"); > - return -ENOMEM; > - } > - master->of_node = dev->of_node; > - list_add(&master->list, &(*iommu)->ctx_list); > - dev_iommu_priv_set(dev, master); > + /* A master describes one device on one IOMMU instance. */ > + list_for_each_entry(master, &(*iommu)->ctx_list, list) > + if (master->of_node == dev->of_node) > + goto add_sid; > + > + master = kzalloc_obj(*master, GFP_ATOMIC); > + if (!master) { > + dev_err(dev, "Failed to allocate iommu_master\n"); > + return -ENOMEM; > } > + master->of_node = dev->of_node; > + list_add(&master->list, &(*iommu)->ctx_list); [Severity: High] Is it possible this change breaks device attachment for shared IOMMUs? By allowing multiple masters to be added to ctx_list, find_iommu_for_dev() and msm_iommu_attach_dev() might fail because they still only check the first entry: drivers/iommu/msm_iommu.c:find_iommu_for_dev() { master = list_first_entry(&iommu->ctx_list, struct msm_iommu_ctx_dev, list); if (master->of_node == dev->of_node) { ... } When a second device probes, it is added to the head of the list. When the first device subsequently tries to attach, it would compare its of_node against the new head (the second device), fail the match, and return -ENODEV. Furthermore, when a match is found in msm_iommu_attach_dev(), it iterates over all masters in ctx_list (even those belonging to other devices): drivers/iommu/msm_iommu.c:msm_iommu_attach_dev() { ... list_for_each_entry(master, &iommu->ctx_list, list) { if (master->num) { dev_err(dev, "domain already attached"); ... } Will this improperly attach all devices sharing the IOMMU to the same domain, breaking domain isolation? [Severity: High] This isn't a bug introduced by this patch, but could calling list_first_entry() on a potentially empty ctx_list cause an out-of-bounds read? During device probe, find_iommu_for_dev() and msm_iommu_attach_dev() iterate over all registered IOMMUs in qcom_iommu_devices and unconditionally call list_first_entry(): drivers/iommu/msm_iommu.c:find_iommu_for_dev() { master = list_first_entry(&iommu->ctx_list, struct msm_iommu_ctx_dev, list); if (master->of_node == dev->of_node) { ... } If an IOMMU has been registered but no devices have been attached to it yet (such as during early bus probing), its ctx_list is empty. list_first_entry on an empty list yields an invalid pointer. Could this lead to a panic via page fault when master->of_node is dereferenced? [Severity: High] This is a pre-existing issue, but does using list_add() here introduce a race condition with concurrent lockless readers? The TLB flush functions (__flush_iotlb and __flush_iotlb_range) iterate over iommu->ctx_list without holding msm_iommu_lock or using RCU primitives: drivers/iommu/msm_iommu.c:__flush_iotlb() { ... list_for_each_entry(master, &iommu->ctx_list, list) SET_CTX_TLBIALL(iommu->base, master->num, 0); ... } At the same time, insert_iommu_master() modifies this same ctx_list using list_add() (a non-RCU primitive) while holding msm_iommu_lock. Since this patch allows multiple masters per IOMMU, can a concurrent TLB flush observe torn pointers or partially initialized nodes if a second device is probed while the first device is actively performing DMA/unmap operations? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
