On 2/13/2024 4:40 PM, Dmitry Baryshkov wrote:
On Tue, 13 Feb 2024 at 12:29, Pratyush Brahma <quic_pbrahma@xxxxxxxxxxx> wrote:
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].
Can you please comment on your thoughts on this as well?
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().
realloc is a basic function. Not understanding it is a significant
problem for the developer.
Hence, a better fix IMO, would be to remove the confusion around the
freed "smmu" ptr in the following way:
Could you please post a proper patch, which can be reviewed and
accepted or declined?
Sure, will do.
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.
None of the other implementations does this. If you are going to fix
qcom implementation, please fix all implementations.
Ohh okay. Wasn't aware that this may be an issue in other
implementations as well.
Will check and raise a formal patch.
However a better
option might be to change arm-smmu to remove devm_krealloc() usage at
all.
Can you please elaborate on your thoughts on how removing devm_krealloc()
usage would be better? Is it because this implementation is error prone
or do you
think this isn't required at all?
I agree on your previous comment that realloc is a basic function and
developers
should understand that before using it. But as you've pointed out that
implementations other than
qcom may also have this issue, I'm inclined to think that the usage of
the api is quite error prone and
there may be some room for improving the usage text perhaps or some
other way.
--
With best wishes
Dmitry
Thanks,
Pratyush
