Re: [PATCH v1 08/10] iommu/arm-smmu-qcom: Merge table from arm-smmu-qcom-debug into match data

[Pratyush Brahma <quic_pbrahma@quicinc.com>]

On 2/13/2024 4:40 PM, Dmitry Baryshkov wrote:
> On Tue, 13 Feb 2024 at 12:29, Pratyush Brahma <quic_pbrahma@quicinc.com> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel