linux-kernel - Re: [PATCH 2/6] iommu/qcom: Write TCR before TTBRs to fix ASID access behavior

Open Source and information security mailing list archives

Message-ID: <edf89f9a-23ce-16ce-33f4-7796ed6bdb4c@arm.com>
Date:   Tue, 31 May 2022 17:26:14 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Will Deacon <will@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>
Cc:     linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        jamipkettunen@...ainline.org, iommu@...ts.linux-foundation.org,
        martin.botka@...ainline.org, ~postmarketos/upstreaming@...ts.sr.ht,
        angelogioacchino.delregno@...ainline.org,
        marijn.suijten@...ainline.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/6] iommu/qcom: Write TCR before TTBRs to fix ASID access
 behavior

On 2022-05-31 16:55, Will Deacon wrote:
> On Fri, May 27, 2022 at 11:28:57PM +0200, Konrad Dybcio wrote:
>> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
>>
>> As also stated in the arm-smmu driver, we must write the TCR before
>> writing the TTBRs, since the TCR determines the access behavior of
>> some fields.
> 
> Where is this stated in the arm-smmu driver?

In arm_smmu_write_context_bank() - IIRC it's mostly about the case where 
if you write a 16-bit ASID to TTBR before setting TCR2.AS you might end 
up losing the top 8 bits of it. However, in the context of a pantomime 
where we just have to pretend to program the "hardware" the way the 
firmware has already programmed it (on pain of getting randomly reset if 
we look at it wrong), I can't imagine it really matters.

Robin.

>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
>> Signed-off-by: Marijn Suijten <marijn.suijten@...ainline.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@...ainline.org>
>> ---
>>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> index 1728d4d7fe25..75f353866c40 100644
>> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> @@ -273,18 +273,18 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>>   			ctx->secure_init = true;
>>   		}
>>   
>> -		/* TTBRs */
>> -		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
>> -				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
>> -				FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
>> -		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
>> -
>>   		/* TCR */
>>   		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
>>   				arm_smmu_lpae_tcr2(&pgtbl_cfg));
>>   		iommu_writel(ctx, ARM_SMMU_CB_TCR,
>>   			     arm_smmu_lpae_tcr(&pgtbl_cfg) | ARM_SMMU_TCR_EAE);
>>   
>> +		/* TTBRs */
>> +		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
>> +				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
>> +				FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
>> +		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
> 
> I'd have thought that SCTLR.M would be clear here, so it shouldn't matter
> what order we write these in.
> 
> Will
> _______________________________________________
> iommu mailing list
> iommu@...ts.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives