| 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
| ||
|
Message-ID: <20150724165325.GC21177@arm.com>
Date: Fri, 24 Jul 2015 17:53:25 +0100
From: Will Deacon <will.deacon@....com>
To: Yong Wu <yong.wu@...iatek.com>
Cc: Joerg Roedel <joro@...tes.org>,
Thierry Reding <treding@...dia.com>,
Mark Rutland <Mark.Rutland@....com>,
Matthias Brugger <matthias.bgg@...il.com>,
Robin Murphy <Robin.Murphy@....com>,
Daniel Kurtz <djkurtz@...gle.com>,
Tomasz Figa <tfiga@...gle.com>,
Lucas Stach <l.stach@...gutronix.de>,
Rob Herring <robh+dt@...nel.org>,
Catalin Marinas <Catalin.Marinas@....com>,
"linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>,
Sasha Hauer <kernel@...gutronix.de>,
"srv_heupstream@...iatek.com" <srv_heupstream@...iatek.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
"pebolle@...cali.nl" <pebolle@...cali.nl>,
"arnd@...db.de" <arnd@...db.de>,
"mitchelh@...eaurora.org" <mitchelh@...eaurora.org>,
"cloud.chou@...iatek.com" <cloud.chou@...iatek.com>,
"frederic.chen@...iatek.com" <frederic.chen@...iatek.com>
Subject: Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table
allocator.
On Fri, Jul 24, 2015 at 06:24:26AM +0100, Yong Wu wrote:
> On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
> > On Thu, Jul 16, 2015 at 10:04:32AM +0100, Yong Wu wrote:
> > > +/* level 2 pagetable */
> > > +#define ARM_SHORT_PTE_TYPE_LARGE BIT(0)
> > > +#define ARM_SHORT_PTE_SMALL_XN BIT(0)
> > > +#define ARM_SHORT_PTE_TYPE_SMALL BIT(1)
> > > +#define ARM_SHORT_PTE_B BIT(2)
> > > +#define ARM_SHORT_PTE_C BIT(3)
> > > +#define ARM_SHORT_PTE_SMALL_TEX0 BIT(6)
> > > +#define ARM_SHORT_PTE_IMPLE BIT(9)
> >
> > This is AP[2] for small pages.
>
> Sorry, In our pagetable bit9 in PGD and PTE is PA[32] that is for the
> dram size over 4G. I didn't care it is different in PTE of the standard
> spec.
> And I don't use the AP[2] currently, so I only delete this line in next
> time.
Is this related to the "special bit". What would be good is a comment
next to the #define for the quirk describing *exactly* that differs in
your implementation. Without that, it's very difficult to know what is
intentional and what is actually broken.
> > > +static arm_short_iopte
> > > +__arm_short_pte_prot(struct arm_short_io_pgtable *data, int prot, bool large)
> > > +{
> > > + arm_short_iopte pteprot;
> > > +
> > > + pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
> > > + pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
> > > + ARM_SHORT_PTE_TYPE_SMALL;
> > > + if (prot & IOMMU_CACHE)
> > > + pteprot |= ARM_SHORT_PTE_B | ARM_SHORT_PTE_C;
> > > + if (prot & IOMMU_WRITE)
> > > + pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
> > > + ARM_SHORT_PTE_SMALL_TEX0;
> >
> > This doesn't make any sense. TEX[2:0] is all about memory attributes, not
> > permissions, so you're making the mapping write-back, write-allocate but
> > that's not what the IOMMU_* values are about.
>
> I will delete it.
Well, can you not control mapping permissions with the AP bits? The idea
of the IOMMU flags are:
IOMMU_CACHE : Install a normal, cacheable mapping (you've got this right)
IOMMU_READ : Allow read access for the device
IOMMU_WRITE : Allow write access for the device
IOMMU_NOEXEC : Disallow execute access for the device
so the caller to iommu_map passes in a bitmap of these, which you need to
encode in the page-table entry.
> > > +static int
> > > +_arm_short_map(struct arm_short_io_pgtable *data,
> > > + unsigned int iova, phys_addr_t paddr,
> > > + arm_short_iopte pgdprot, arm_short_iopte pteprot,
> > > + bool large)
> > > +{
> > > + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> > > + arm_short_iopte *pgd = data->pgd, *pte;
> > > + void *cookie = data->iop.cookie, *pte_va;
> > > + unsigned int ptenr = large ? 16 : 1;
> > > + int i, quirk = data->iop.cfg.quirks;
> > > + bool ptenew = false;
> > > +
> > > + pgd += ARM_SHORT_PGD_IDX(iova);
> > > +
> > > + if (!pteprot) { /* section or supersection */
> > > + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK)
> > > + pgdprot &= ~ARM_SHORT_PGD_SECTION_XN;
> > > + pte = pgd;
> > > + pteprot = pgdprot;
> > > + } else { /* page or largepage */
> > > + if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > > + if (large) { /* special Bit */
> >
> > This definitely needs a better comment! What exactly are you doing here
> > and what is that quirk all about?
>
> I use this quirk is for MTK Special Bit as we don't have the XN bit in
> pagetable.
I'm still not really clear about what this is.
> > > + if (!(*pgd)) {
> > > + pte_va = kmem_cache_zalloc(data->ptekmem, GFP_ATOMIC);
> > > + if (unlikely(!pte_va))
> > > + return -ENOMEM;
> > > + ptenew = true;
> > > + *pgd = virt_to_phys(pte_va) | pgdprot;
> > > + kmemleak_ignore(pte_va);
> > > + tlb->flush_pgtable(pgd, sizeof(*pgd), cookie);
> >
> > I think you need to flush this before it becomes visible to the walker.
>
> I have flushed pgtable here, Do you meaning flush tlb here?
No. afaict, you allocate the pte table using kmem_cache_zalloc but you never
flush it. However, you update the pgd to point at this table, so the walker
can potentially see garbage instead of the zeroed entries.
Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists