linux-kernel - Re: [PATCH v7 13/23] iommu/smmuv3: Implement attach/detach_pasid

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]

Date:   Mon, 13 May 2019 13:04:24 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Auger Eric <eric.auger@...hat.com>, eric.auger.pro@...il.com,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, kvmarm@...ts.cs.columbia.edu, joro@...tes.org,
        alex.williamson@...hat.com, jacob.jun.pan@...ux.intel.com,
        yi.l.liu@...el.com, jean-philippe.brucker@....com,
        will.deacon@....com
Cc:     kevin.tian@...el.com, ashok.raj@...el.com, marc.zyngier@....com,
        christoffer.dall@....com, peter.maydell@...aro.org,
        vincent.stehle@....com
Subject: Re: [PATCH v7 13/23] iommu/smmuv3: Implement
 attach/detach_pasid_table

On 10/05/2019 15:35, Auger Eric wrote:
> Hi Robin,
> 
> On 5/8/19 4:38 PM, Robin Murphy wrote:
>> On 08/04/2019 13:19, Eric Auger wrote:
>>> On attach_pasid_table() we program STE S1 related info set
>>> by the guest into the actual physical STEs. At minimum
>>> we need to program the context descriptor GPA and compute
>>> whether the stage1 is translated/bypassed or aborted.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@...hat.com>
>>>
>>> ---
>>> v6 -> v7:
>>> - check versions and comment the fact we don't need to take
>>>     into account s1dss and s1fmt
>>> v3 -> v4:
>>> - adapt to changes in iommu_pasid_table_config
>>> - different programming convention at s1_cfg/s2_cfg/ste.abort
>>>
>>> v2 -> v3:
>>> - callback now is named set_pasid_table and struct fields
>>>     are laid out differently.
>>>
>>> v1 -> v2:
>>> - invalidate the STE before changing them
>>> - hold init_mutex
>>> - handle new fields
>>> ---
>>>    drivers/iommu/arm-smmu-v3.c | 121 ++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 121 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>>> index e22e944ffc05..1486baf53425 100644
>>> --- a/drivers/iommu/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm-smmu-v3.c
>>> @@ -2207,6 +2207,125 @@ static void arm_smmu_put_resv_regions(struct
>>> device *dev,
>>>            kfree(entry);
>>>    }
>>>    +static int arm_smmu_attach_pasid_table(struct iommu_domain *domain,
>>> +                       struct iommu_pasid_table_config *cfg)
>>> +{
>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>> +    struct arm_smmu_master_data *entry;
>>> +    struct arm_smmu_s1_cfg *s1_cfg;
>>> +    struct arm_smmu_device *smmu;
>>> +    unsigned long flags;
>>> +    int ret = -EINVAL;
>>> +
>>> +    if (cfg->format != IOMMU_PASID_FORMAT_SMMUV3)
>>> +        return -EINVAL;
>>> +
>>> +    if (cfg->version != PASID_TABLE_CFG_VERSION_1 ||
>>> +        cfg->smmuv3.version != PASID_TABLE_SMMUV3_CFG_VERSION_1)
>>> +        return -EINVAL;
>>> +
>>> +    mutex_lock(&smmu_domain->init_mutex);
>>> +
>>> +    smmu = smmu_domain->smmu;
>>> +
>>> +    if (!smmu)
>>> +        goto out;
>>> +
>>> +    if (!((smmu->features & ARM_SMMU_FEAT_TRANS_S1) &&
>>> +          (smmu->features & ARM_SMMU_FEAT_TRANS_S2))) {
>>> +        dev_info(smmu_domain->smmu->dev,
>>> +             "does not implement two stages\n");
>>> +        goto out;
>>> +    }
>>
>> That check is redundant (and frankly looks a little bit spammy). If the
>> one below is not enough, there is a problem elsewhere - if it's possible
>> for smmu_domain->stage to ever get set to ARM_SMMU_DOMAIN_NESTED without
>> both stages of translation present, we've already gone fundamentally wrong.
> 
> Makes sense. Moved that check to arm_smmu_domain_finalise() instead and
> remove redundant ones.

Urgh, I forgot exactly how the crazy domain-allocation dance worked, 
such that we're not in a position to refuse the domain_set_attr() call 
itself, but that does sound like the best compromise for now.

Thanks,
Robin.