linux-kernel - Re: [PATCH] iommu/arm-smmu-v3: Simplify stage selection logic

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a8623bd8-1df4-42b7-4993-006316594818@arm.com>
Date:   Thu, 17 Aug 2023 19:19:52 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Michael Shavit <mshavit@...gle.com>
Cc:     iommu@...ts.linux.dev, Jason Gunthorpe <jgg@...pe.ca>,
        Jean-Philippe Brucker <jean-philippe@...aro.org>,
        Joerg Roedel <joro@...tes.org>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        Nicolin Chen <nicolinc@...dia.com>,
        Tomas Krcka <krckatom@...zon.de>,
        Will Deacon <will@...nel.org>,
        Yicong Yang <yangyicong@...ilicon.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iommu/arm-smmu-v3: Simplify stage selection logic

On 2023-08-17 18:06, Michael Shavit wrote:
> On Fri, Aug 18, 2023 at 12:35 AM Robin Murphy <robin.murphy@....com> wrote:
>>
>> The reason it's like this is because of arm_smmu_enable_nesting(), which
>> *is* the additional thing that's going on with the stage selection logic.
>>
>> Thanks,
>> Robin.
> 
> Right, but arm_smmu_enable_nesting isn't involved in this computation
> at this point in the flow.
> 
> arm_smmu_enable_nesting returns early if smmu_domain->smmu isn't set,
> and smmu_domain->smmu is only set after arm_smmu_domain_finalise.
> So at this point, smmu_domain->stage is being initialized for the
> first time. If this code is responsible for handling some special
> nesting case, then it's probably not working as intended.

I think you may have misread that code...

Anyway, the point of the logic here is that it is not "selection", it 
is, as the comment says, "restriction" - i.e. it is checking that the 
already-selected stage is actually supported, and coercing it if not. 
The default selection for a newly-allocated domain is always implicitly 
ARM_SMMU_DOMAIN_S1 (which is explicitly defined as 0 to convey that 
significance), but it may be set to ARM_SMMU_DOMAIN_NESTED before the 
first attach finalises the pagetable format.

Obviously this could be clearer, especially for anyone not so familiar 
with all the history, but at this point I honestly don't think it's 
worth doing anything without completely ripping out 
arm_smmu_enable_nesting() as well. Jason already had a patch a while 
ago, and my bus rework is now also very close to the point of finally 
fixing iommu_domain_alloc() to be able to return working domains, such 
that all the "domain_finalise" bodges go away and that whole "modify the 
domain between allocation and attach" paradigm is no longer valid at all.

By this point I'm not too fussed about breaking the current meaning of 
ARM_SMMU_DOMAIN_NESTED any more. But what I definitely don't want to do 
is have a change like this which subtly but decisively breaks it while 
still leaving all the now-dead code in place ;)

Thanks,
Robin.