linux-kernel - Re: [PATCH v1] iommu/arm-smmu-v3: Allow default substream bypass with a pasid support

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: <ZJsXLFtH8WkRK41w@Asurada-Nvidia>
Date:   Tue, 27 Jun 2023 10:06:52 -0700
From:   Nicolin Chen <nicolinc@...dia.com>
To:     Robin Murphy <robin.murphy@....com>
CC:     <will@...nel.org>, <jgg@...dia.com>, <joro@...tes.org>,
        <shameerali.kolothum.thodi@...wei.com>, <yangyicong@...ilicon.com>,
        <jean-philippe@...aro.org>, <linux-arm-kernel@...ts.infradead.org>,
        <iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] iommu/arm-smmu-v3: Allow default substream bypass
 with a pasid support

Hi Robin,

On Tue, Jun 27, 2023 at 10:00:18AM +0100, Robin Murphy wrote:
> On 2023-06-27 04:33, Nicolin Chen wrote:
> > When an iommu_domain is set to IOMMU_DOMAIN_IDENTITY, the driver would
> > skip the allocation of a CD table and set the CONFIG field of the STE
> > to STRTAB_STE_0_CFG_BYPASS. This works well for devices that only have
> > one substream, i.e. PASID disabled.
> > 
> > However, there could be a use case, for a pasid capable device, that
> > allows bypassing the translation at the default substream while still
> > enabling the pasid feature, which means the driver should not skip the
> > allocation of a CD table nor simply bypass the CONFIG field. Instead,
> > the S1DSS field should be set to STRTAB_STE_1_S1DSS_BYPASS and the
> > SHCFG field should be set to STRTAB_STE_1_SHCFG_INCOMING.
> > 
> > Add s1dss and shcfg in struct arm_smmu_s1_cfg, to allow configurations
> > in the finalise() to support that use case. Then, set them accordingly
> > depending on the iommu_domain->type and the master->ssid_bits.
> > 
> > Also, add STRTAB_STE_1_SHCFG_NONSHAREABLE of the default configuration
> > to distinguish from STRTAB_STE_1_SHCFG_INCOMING of the bypass one.
> 
> Why? The "default configuration" is that the S1 shareability attribute
> is determined by the S1 translation itself, so the incoming value is
> irrelevant.

That was for a consistency since the driver set the SHCFG field
to 0x0 (STRTAB_STE_1_SHCFG_NONSHAREABLE). I was not quite sure,
in a long run, if leaving an uncleared s1_cfg->shcfg potentially
can result in an unexpected behavior if it's passed in the STE.
Yet, we could be seemingly sure that the !IOMMU_DOMAIN_IDENTITY
means the S1 translation must be enabled and so the SHCFG would
be irrelevant?

If so, I make make it:

+	if (smmu_domain->domain.type == IOMMU_DOMAIN_IDENTITY) {
+		cfg->s1dss = STRTAB_STE_1_S1DSS_BYPASS;
+		cfg->shcfg = STRTAB_STE_1_SHCFG_INCOMING;
+	} else {
+		cfg->s1dss = STRTAB_STE_1_S1DSS_SSID0;
+	}

> > @@ -2198,7 +2206,11 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain,
> >       struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> >       struct arm_smmu_device *smmu = smmu_domain->smmu;
> > 
> > -     if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> > +     /*
> > +      * A master with a pasid capability might need a CD table, so only set
> > +      * ARM_SMMU_DOMAIN_BYPASS if IOMMU_DOMAIN_IDENTITY and non-pasid master
> > +      */
> > +     if (domain->type == IOMMU_DOMAIN_IDENTITY && !master->ssid_bits) {
> >               smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> >               return 0;
> >       }
> 
> This means we'll now go on to allocate a pagetable for an identity
> domain, which doesn't seem ideal :/

Do you suggest to bypass alloc_io_pgtable_ops()? That would zero
out the TCR fields in the CD. Not sure if it'd work seamlessly,
but I can give it a try.

Thanks
Nic