| Message ID | 20200407183742.4344-18-joro@8bytes.org |
|---|---|
| State | New |
| Headers | show |
| Series | iommu: Move iommu_group setup to IOMMU core code | expand |
On 2020-04-08 3:37 pm, Joerg Roedel wrote: > Hi Robin, > > thanks for looking into this. > > On Wed, Apr 08, 2020 at 01:09:40PM +0100, Robin Murphy wrote: >> For a hot-pluggable bus where logical devices may share Stream IDs (like >> fsl-mc), this could happen: >> >> create device A >> iommu_probe_device(A) >> iommu_device_group(A) -> alloc group X >> create device B >> iommu_probe_device(B) >> iommu_device_group(A) -> lookup returns group X >> ... >> iommu_remove_device(A) >> delete device A >> create device C >> iommu_probe_device(C) >> iommu_device_group(C) -> use-after-free of A >> >> Preserving the logical behaviour here would probably look *something* like >> the mangled diff below, but I haven't thought it through 100%. > > Yeah, I think you are right. How about just moving the loop which sets > s2crs[idx].group to arm_smmu_device_group()? In that case I can drop > this patch and leave the group pointer in place. Isn't that exactly what I suggested? :) I don't recall for sure, but knowing me, that bit of group bookkeeping is only where it currently is because it cheekily saves iterating the IDs a second time. I don't think there's any technical reason. Robin.
On Wed, Apr 08, 2020 at 04:07:33PM +0100, Robin Murphy wrote: > On 2020-04-08 3:37 pm, Joerg Roedel wrote: > Isn't that exactly what I suggested? :) Okay, I dropped this patch and updated the next one. > I don't recall for sure, but knowing me, that bit of group bookkeeping is > only where it currently is because it cheekily saves iterating the IDs a > second time. I don't think there's any technical reason. I leave it up to you to make any changes on that :) Updated patch below. I also noticed that I deleted too much from arm-smmu-v3 in the previous version, fixed that too.
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index a6a5796e9c41..3493501d8b2c 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -69,7 +69,7 @@ MODULE_PARM_DESC(disable_bypass, "Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU."); struct arm_smmu_s2cr { - struct iommu_group *group; + struct device *dev; int count; enum arm_smmu_s2cr_type type; enum arm_smmu_s2cr_privcfg privcfg; @@ -1100,7 +1100,7 @@ static int arm_smmu_master_alloc_smes(struct device *dev) /* It worked! Now, poke the actual hardware */ for_each_cfg_sme(cfg, fwspec, i, idx) { arm_smmu_write_sme(smmu, idx); - smmu->s2crs[idx].group = group; + smmu->s2crs[idx].dev = dev; } mutex_unlock(&smmu->stream_map_mutex); @@ -1495,11 +1495,15 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev) int i, idx; for_each_cfg_sme(cfg, fwspec, i, idx) { - if (group && smmu->s2crs[idx].group && - group != smmu->s2crs[idx].group) + struct iommu_group *idx_grp = NULL; + + if (smmu->s2crs[idx].dev) + idx_grp = smmu->s2crs[idx].dev->iommu_group; + + if (group && idx_grp && group != idx_grp) return ERR_PTR(-EINVAL); - group = smmu->s2crs[idx].group; + group = idx_grp; } if (group)