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]. > > 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? > > 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. However a better option might be to change arm-smmu to remove devm_krealloc() usage at all. -- With best wishes Dmitry
