Hi
Patch [1] introduces a use after free bug in the function
"qcom_smmu_create()" in file: drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
wherein devm_krealloc() frees the old pointer marked by "smmu" but it is
still being accessed later in qcom_smmu_impl_data() in the same function
as:
qsmmu->cfg = qcom_smmu_impl_data(smmu);
The current patchset [2] implicitly fixes this issue as it doesn't
access the freed ptr in the line:
qsmmu->cfg = data->cfg;
Hence, can this patchset[2] be propagated to branches where patchset[1]
has been propagated already? The bug is currently present in all branches
that have patchset[1] but do not have patchset[2].
RFC:
This bug would be reintroduced if patchset [3] is accepted. This makes
the path prone to such errors as it relies on the
developer's understanding on the internal implementation of devm_krealloc().
Hence, a better fix IMO, would be to remove the confusion around the
freed "smmu" ptr in the following way:
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 549ae4dba3a6..6dd142ce75d1 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -463,11 +463,12 @@ static struct arm_smmu_device
*qcom_smmu_create(struct arm_smmu_device *smmu,
qsmmu = devm_krealloc(smmu->dev, smmu, sizeof(*qsmmu), GFP_KERNEL);
if (!qsmmu)
return ERR_PTR(-ENOMEM);
+ smmu = &qsmmu->smmu;
qsmmu->smmu.impl = impl;
qsmmu->cfg = data->cfg;
- return &qsmmu->smmu;
+ return smmu;
}
This is similar to the patch[4] which I've sent in-reply-to patch[3].
Will send a formal patch if you think this approach is better.
Please let me know your thoughts.
Thanks,
Pratyush
[1]
https://lore.kernel.org/all/20220708094230.4349-1-quic_saipraka@xxxxxxxxxxx/
[2]
https://lore.kernel.org/all/20221114170635.1406534-9-dmitry.baryshkov@xxxxxxxxxx/
[3]
https://lore.kernel.org/all/20240201210529.7728-4-quic_c_gdjako@xxxxxxxxxxx/
[4]
https://lore.kernel.org/all/20240213062608.13018-1-quic_pbrahma@xxxxxxxxxxx/
