The ARM MMU-500 implements a Translation Buffer Unit (TBU) for each
connected master besides a single TCU which controls and manages the
address translations.
Allow the Qualcomm SMMU driver to probe for any TBU devices that can
provide additional debug features like triggering transactions, logging
outstanding transactions, snapshot capture etc. The primary use-case
would be to get information from a TBU and print it during a context
fault.
Signed-off-by: Georgi Djakov <[email protected]>
---
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 9 +++++++++
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h | 4 +++-
2 files changed, 12 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 8b04ece00420..ca806644e6eb 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -1,12 +1,14 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
* Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved
*/
#include <linux/acpi.h>
#include <linux/adreno-smmu-priv.h>
#include <linux/delay.h>
#include <linux/of_device.h>
+#include <linux/of_platform.h>
#include <linux/firmware/qcom/qcom_scm.h>
#include "arm-smmu.h"
@@ -446,6 +448,7 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
const struct device_node *np = smmu->dev->of_node;
const struct arm_smmu_impl *impl;
struct qcom_smmu *qsmmu;
+ int ret;
if (!data)
return ERR_PTR(-EINVAL);
@@ -469,6 +472,12 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
qsmmu->smmu.impl = impl;
qsmmu->cfg = data->cfg;
+ INIT_LIST_HEAD(&qsmmu->tbu_list);
+ mutex_init(&qsmmu->tbu_list_lock);
+ ret = devm_of_platform_populate(smmu->dev);
+ if (ret)
+ return ERR_PTR(ret);
+
return &qsmmu->smmu;
}
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
index 593910567b88..77e5becc2482 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
- * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2022-2024, Qualcomm Innovation Center, Inc. All rights reserved.
*/
#ifndef _ARM_SMMU_QCOM_H
@@ -12,6 +12,8 @@ struct qcom_smmu {
bool bypass_quirk;
u8 bypass_cbndx;
u32 stall_enabled;
+ struct mutex tbu_list_lock; /* protects tbu_list */
+ struct list_head tbu_list;
};
enum qcom_smmu_impl_reg_offset {
Hi
The following patch would introduce a use-after-free bug which was found
during KASAN testing on qcm6490 with the patch.
diff
<https://lore.kernel.org/all/[email protected]/#iZ2e.:20240201210529.7728-4-quic_c_gdjako::40quicinc.com:1drivers:iommu:arm:arm-smmu:arm-smmu-qcom.c>
--git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c index
8b04ece00420..ca806644e6eb 100644 ---
a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c +++
b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c @@ -1,12 +1,14 @@ // SPDX-License-Identifier: GPL-2.0-only
/*
* Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved */
#include <linux/acpi.h>
#include <linux/adreno-smmu-priv.h>
#include <linux/delay.h>
#include <linux/of_device.h>
+#include <linux/of_platform.h> #include <linux/firmware/qcom/qcom_scm.h>
#include "arm-smmu.h"
@@ -446,6 +448,7 @@ static struct arm_smmu_device
*qcom_smmu_create(struct arm_smmu_device *smmu, const struct device_node *np = smmu->dev->of_node;
const struct arm_smmu_impl *impl;
struct qcom_smmu *qsmmu;
+ int ret;
if (!data)
return ERR_PTR(-EINVAL);
@@ -469,6 +472,12 @@ static struct arm_smmu_device
*qcom_smmu_create(struct arm_smmu_device *smmu, qsmmu->smmu.impl = impl;
qsmmu->cfg = data->cfg;
+ INIT_LIST_HEAD(&qsmmu->tbu_list); + mutex_init(&qsmmu->tbu_list_lock);
+ ret = devm_of_platform_populate(smmu->dev); // smmu has been freed by
devm_krealloc() above but is being accessed here again later. This
causes use-after-free bug. + if (ret) + return ERR_PTR(ret); + return &qsmmu->smmu;
}
Can it be done like below?
qsmmu->smmu.impl = impl;
qsmmu->cfg = data->cfg;
+ INIT_LIST_HEAD(&qsmmu->tbu_list); + mutex_init(&qsmmu->tbu_list_lock);
+ ret = devm_of_platform_populate(qsmmu->smmu.dev);// Using the struct
to which smmu was copied instead of freed ptr. Thanks, Pratyush
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 <[email protected]>
---
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);
- return &qsmmu->smmu;
+ return smmu;
}
/* Implementation Defined Register Space 0 register offsets */
--
2.17.1
On Tue, 13 Feb 2024 at 08:27, Pratyush Brahma <[email protected]> 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 <[email protected]>
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
>
>
--
With best wishes
Dmitry
On 2/13/2024 1:36 PM, Dmitry Baryshkov wrote:
> On Tue, 13 Feb 2024 at 08:27, Pratyush Brahma <[email protected]> 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 <[email protected]>
> 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/[email protected]
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
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
>> <[email protected]> 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 <[email protected]>
>> 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/[email protected]
> 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
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 <[email protected]> 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 <[email protected]>
>> 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