| Message ID | 20240213062608.13018-1-quic_pbrahma@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] iommu/arm-smmu-qcom: Fix use-after-free issue in qcom_smmu_create() | expand |
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 > >
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
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
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 --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 */
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(-)