diff mbox series

[1/1] iommu/arm-smmu-qcom: Fix use-after-free issue in qcom_smmu_create()

Message ID 20240213062608.13018-1-quic_pbrahma@quicinc.com
State New
Headers
Series [1/1] iommu/arm-smmu-qcom: Fix use-after-free issue in qcom_smmu_create() |

Commit Message

Pratyush Brahma Feb. 13, 2024, 6:26 a.m. UTC 
  Currently, during arm smmu probe, struct arm_smmu_device pointer
is allocated. The pointer is reallocated to a new struct qcom_smmu in
qcom_smmu_create() with devm_krealloc() which frees the smmu device
after copying the data into the new pointer.

The freed pointer is then passed again in devm_of_platform_populate()
inside qcom_smmu_create() which causes a use-after-free issue.

Fix the use-after-free issue by reassigning the old pointer to
the new pointer where the struct was copied by devm_krealloc().

Signed-off-by: Pratyush Brahma <quic_pbrahma@quicinc.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Dmitry Baryshkov Feb. 13, 2024, 8:06 a.m. UTC | #1 
On Tue, 13 Feb 2024 at 08:27, Pratyush Brahma <quic_pbrahma@quicinc.com> wrote:
>
> Currently, during arm smmu probe, struct arm_smmu_device pointer
> is allocated. The pointer is reallocated to a new struct qcom_smmu in
> qcom_smmu_create() with devm_krealloc() which frees the smmu device
> after copying the data into the new pointer.
>
> The freed pointer is then passed again in devm_of_platform_populate()
> inside qcom_smmu_create() which causes a use-after-free issue.
>
> Fix the use-after-free issue by reassigning the old pointer to
> the new pointer where the struct was copied by devm_krealloc().
>
> Signed-off-by: Pratyush Brahma <quic_pbrahma@quicinc.com>

Missing Fixes tag.

> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index ed5ed5da7740..49eaeed6a91c 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -710,6 +710,7 @@ 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->data = data;
> @@ -719,7 +720,7 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
>         if (ret)
>                 return ERR_PTR(ret);

What is the tree that you have been developing this against? I don't
see this part of the code in the linux-next.

>
> -       return &qsmmu->smmu;
> +       return smmu;
>  }
>
>  /* Implementation Defined Register Space 0 register offsets */
> --
> 2.17.1
>
>
Pratyush Brahma Feb. 13, 2024, 8:17 a.m. UTC | #2 
On 2/13/2024 1:36 PM, Dmitry Baryshkov wrote:
> On Tue, 13 Feb 2024 at 08:27, Pratyush Brahma <quic_pbrahma@quicinc.com> wrote:
>> Currently, during arm smmu probe, struct arm_smmu_device pointer
>> is allocated. The pointer is reallocated to a new struct qcom_smmu in
>> qcom_smmu_create() with devm_krealloc() which frees the smmu device
>> after copying the data into the new pointer.
>>
>> The freed pointer is then passed again in devm_of_platform_populate()
>> inside qcom_smmu_create() which causes a use-after-free issue.
>>
>> Fix the use-after-free issue by reassigning the old pointer to
>> the new pointer where the struct was copied by devm_krealloc().
>>
>> Signed-off-by: Pratyush Brahma <quic_pbrahma@quicinc.com>
> Missing Fixes tag.
Haven't added as the patchset in-reply-to hasn't been merged to 
linux-next. Please refer my next reply.
>
>> ---
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> index ed5ed5da7740..49eaeed6a91c 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -710,6 +710,7 @@ 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->data = data;
>> @@ -719,7 +720,7 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
>>          if (ret)
>>                  return ERR_PTR(ret);
> What is the tree that you have been developing this against? I don't
> see this part of the code in the linux-next.
This is in reply to the patchset at: 
https://lore.kernel.org/all/20240201210529.7728-4-quic_c_gdjako@quicinc.com
The aforementioned patchset introduces this bug. This is a suggested fix 
to the bug.
>> -       return &qsmmu->smmu;
>> +       return smmu;
>>   }
>>
>>   /* Implementation Defined Register Space 0 register offsets */
>> --
>> 2.17.1
>>
>>
Thanks,
Pratyush
Robin Murphy Feb. 13, 2024, 11:36 a.m. UTC | #3 
On 2024-02-13 8:17 am, Pratyush Brahma wrote:
> 
> On 2/13/2024 1:36 PM, Dmitry Baryshkov wrote:
>> On Tue, 13 Feb 2024 at 08:27, Pratyush Brahma 
>> <quic_pbrahma@quicinc.com> wrote:
>>> Currently, during arm smmu probe, struct arm_smmu_device pointer
>>> is allocated. The pointer is reallocated to a new struct qcom_smmu in
>>> qcom_smmu_create() with devm_krealloc() which frees the smmu device
>>> after copying the data into the new pointer.
>>>
>>> The freed pointer is then passed again in devm_of_platform_populate()
>>> inside qcom_smmu_create() which causes a use-after-free issue.
>>>
>>> Fix the use-after-free issue by reassigning the old pointer to
>>> the new pointer where the struct was copied by devm_krealloc().
>>>
>>> Signed-off-by: Pratyush Brahma <quic_pbrahma@quicinc.com>
>> Missing Fixes tag.
> Haven't added as the patchset in-reply-to hasn't been merged to 
> linux-next. Please refer my next reply.
>>
>>> ---
>>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> index ed5ed5da7740..49eaeed6a91c 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> @@ -710,6 +710,7 @@ 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->data = data;
>>> @@ -719,7 +720,7 @@ static struct arm_smmu_device 
>>> *qcom_smmu_create(struct arm_smmu_device *smmu,
>>>          if (ret)
>>>                  return ERR_PTR(ret);
>> What is the tree that you have been developing this against? I don't
>> see this part of the code in the linux-next.
> This is in reply to the patchset at: 
> https://lore.kernel.org/all/20240201210529.7728-4-quic_c_gdjako@quicinc.com
> The aforementioned patchset introduces this bug. This is a suggested fix 
> to the bug.

Unless you are the 0-day bot, please just point out bugs in under-review 
patches via regular review comments rather than sending patches for 
unmerged patches.

There is nothing to fix in mainline, and as I commented on the binding 
patch I'm not sure I agree with the fundamental premise for touching 
qcom_smmu_create() in this series at all.

Thanks,
Robin.

>>> -       return &qsmmu->smmu;
>>> +       return smmu;
>>>   }
>>>
>>>   /* Implementation Defined Register Space 0 register offsets */
>>> -- 
>>> 2.17.1
>>>
>>>
> Thanks,
> Pratyush
Krzysztof Kozlowski Feb. 29, 2024, 5:57 p.m. UTC | #4 
On 13/02/2024 09:17, Pratyush Brahma wrote:
> 
> On 2/13/2024 1:36 PM, Dmitry Baryshkov wrote:
>> On Tue, 13 Feb 2024 at 08:27, Pratyush Brahma <quic_pbrahma@quicinc.com> wrote:
>>> Currently, during arm smmu probe, struct arm_smmu_device pointer
>>> is allocated. The pointer is reallocated to a new struct qcom_smmu in
>>> qcom_smmu_create() with devm_krealloc() which frees the smmu device
>>> after copying the data into the new pointer.
>>>
>>> The freed pointer is then passed again in devm_of_platform_populate()
>>> inside qcom_smmu_create() which causes a use-after-free issue.
>>>
>>> Fix the use-after-free issue by reassigning the old pointer to
>>> the new pointer where the struct was copied by devm_krealloc().
>>>
>>> Signed-off-by: Pratyush Brahma <quic_pbrahma@quicinc.com>
>> Missing Fixes tag.
> Haven't added as the patchset in-reply-to hasn't been merged to 
> linux-next. Please refer my next reply.

Why do you send patches for work being reviewed? Just perform the
review. It looks like you deliberately want to apply bad code just to
fix it a second later!

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index ed5ed5da7740..49eaeed6a91c 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -710,6 +710,7 @@  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->data = data;
@@ -719,7 +720,7 @@  static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
 	if (ret)
 		return ERR_PTR(ret);
 
-	return &qsmmu->smmu;
+	return smmu;
 }
 
 /* Implementation Defined Register Space 0 register offsets */