| Message ID | 1405954040-30399-6-git-send-email-daniel.thompson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 21, 2014 at 03:47:16PM +0100, Daniel Thompson wrote: > From: Marek Vasut <marex@denx.de> > > Add new device type, MT_DEVICE_NS. This type sets the NS bit in L1 PTE [1]. > Accesses to a memory region which is mapped this way generate non-secure > access to that memory area. One must be careful here, since the NS bit is > only available in L1 PTE, therefore when creating the mapping, the mapping > must be at least 1 MiB big and must be aligned to 1 MiB. If that condition > was false, the kernel would use regular L2 page mapping for this area instead > and the NS bit setting would be ineffective. Right, so this says that PTE mappings are not permissible. > + [MT_DEVICE_NS] = { /* Non-secure accesses from secure mode */ > + .prot_pte = PROT_PTE_DEVICE | L_PTE_MT_DEV_SHARED | > + L_PTE_SHARED, > + .prot_l1 = PMD_TYPE_TABLE, However, by filling in prot_pte and prot_l1, you're telling the code that it /can/ setup such a mapping. This is screwed. If you want to deny anything but section mappings (because they don't work) then you omit prot_pte and prot_l1. With those omitted, if someone tries to abuse this mapping type, then this check in create_mapping() will trigger: if (type->prot_l1 == 0 && ((addr | phys | length) & ~SECTION_MASK)) { printk(KERN_WARNING "BUG: map for 0x%08llx at 0x%08lx can not " "be mapped using pages, ignoring.\n", (long long)__pfn_to_phys(md->pfn), addr); return; } ioremap doesn't have that check; it assumes that it will always be setting up PTE mappings via ioremap_page_range(). In fact, on many platforms that's the only option. So making this interface available via ioremap() seems pointless - but more importantly it's extremely error-prone. So, MT_DEVICE_NS shouldn't be using 4 at all, shouldn't be in asm/io.h, but should be with the private MT_* definitions in map.h.
On 21/07/14 17:46, Russell King - ARM Linux wrote: > On Mon, Jul 21, 2014 at 03:47:16PM +0100, Daniel Thompson wrote: >> From: Marek Vasut <marex@denx.de> >> >> Add new device type, MT_DEVICE_NS. This type sets the NS bit in L1 PTE [1]. >> Accesses to a memory region which is mapped this way generate non-secure >> access to that memory area. One must be careful here, since the NS bit is >> only available in L1 PTE, therefore when creating the mapping, the mapping >> must be at least 1 MiB big and must be aligned to 1 MiB. If that condition >> was false, the kernel would use regular L2 page mapping for this area instead >> and the NS bit setting would be ineffective. > > Right, so this says that PTE mappings are not permissible. > >> + [MT_DEVICE_NS] = { /* Non-secure accesses from secure mode */ >> + .prot_pte = PROT_PTE_DEVICE | L_PTE_MT_DEV_SHARED | >> + L_PTE_SHARED, >> + .prot_l1 = PMD_TYPE_TABLE, > > However, by filling in prot_pte and prot_l1, you're telling the code that > it /can/ setup such a mapping. This is screwed. I'll fix this. > If you want to deny anything but section mappings (because they don't work) > then you omit prot_pte and prot_l1. With those omitted, if someone tries > to abuse this mapping type, then this check in create_mapping() will > trigger: > > if (type->prot_l1 == 0 && ((addr | phys | length) & ~SECTION_MASK)) { > printk(KERN_WARNING "BUG: map for 0x%08llx at 0x%08lx can not " > "be mapped using pages, ignoring.\n", > (long long)__pfn_to_phys(md->pfn), addr); > return; > } > > ioremap doesn't have that check; it assumes that it will always be setting > up PTE mappings via ioremap_page_range(). In fact, on many platforms > that's the only option. I have proposed a patch (which I just noticed is currently *really* broken but ignore that for now) to prevent the fallback to ioremap_page_range(). As you say this leaves nothing but the lookup in the static mappings for many platforms. That patches looks at PMD_SECT_NS directly but could be changed to zero check ->prot_l1 instead. That removes the danger of spuriously getting bad mappings but is certainly not elegant. > So making this interface available via ioremap() seems pointless - but > more importantly it's extremely error-prone. So, MT_DEVICE_NS shouldn't > be using 4 at all, shouldn't be in asm/io.h, but should be with the > private MT_* definitions in map.h. I wanted to use ioremap() because it allows platform neutral code in the GIC driver to look up a staticly configured non-secure aliased mapping for the GIC (if it exists). Also given the mapping is used for register I/O ioremap() also felt "right". Is new API better? A very thin wrapper around find_static_vm_paddr()? I guess the best thing would be to allocate the mapping dynamically. It might be possible for __arm_ioremap_pfn_caller() to change the NS flag in the first-level table after allocating a naturally aligned 1MB vm_area and before updating the second-level? We are not required to use sections, however all pages that share a L1 entry get the same security flags. Daniel.
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index 3d23418..22765e0 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -125,8 +125,10 @@ static inline u32 __raw_readl(const volatile void __iomem *addr) #define MT_DEVICE_NONSHARED 1 #define MT_DEVICE_CACHED 2 #define MT_DEVICE_WC 3 +#define MT_DEVICE_NS 4 + /* - * types 4 onwards can be found in asm/mach/map.h and are undefined + * types 5 onwards can be found in asm/mach/map.h and are undefined * for ioremap */ @@ -343,6 +345,7 @@ extern void _memset_io(volatile void __iomem *, int, size_t); #define ioremap_nocache(cookie,size) __arm_ioremap((cookie), (size), MT_DEVICE) #define ioremap_cache(cookie,size) __arm_ioremap((cookie), (size), MT_DEVICE_CACHED) #define ioremap_wc(cookie,size) __arm_ioremap((cookie), (size), MT_DEVICE_WC) +#define ioremap_ns(cookie,size) __arm_ioremap((cookie), (size), MT_DEVICE_NS) #define iounmap __arm_iounmap /* diff --git a/arch/arm/include/asm/mach/map.h b/arch/arm/include/asm/mach/map.h index f98c7f3..42be265 100644 --- a/arch/arm/include/asm/mach/map.h +++ b/arch/arm/include/asm/mach/map.h @@ -21,9 +21,9 @@ struct map_desc { unsigned int type; }; -/* types 0-3 are defined in asm/io.h */ +/* types 0-4 are defined in asm/io.h */ enum { - MT_UNCACHED = 4, + MT_UNCACHED = 5, MT_CACHECLEAN, MT_MINICLEAN, MT_LOW_VECTORS, diff --git a/arch/arm/include/asm/pgtable-2level-hwdef.h b/arch/arm/include/asm/pgtable-2level-hwdef.h index 5cfba15..d24e7ea 100644 --- a/arch/arm/include/asm/pgtable-2level-hwdef.h +++ b/arch/arm/include/asm/pgtable-2level-hwdef.h @@ -36,6 +36,7 @@ #define PMD_SECT_S (_AT(pmdval_t, 1) << 16) /* v6 */ #define PMD_SECT_nG (_AT(pmdval_t, 1) << 17) /* v6 */ #define PMD_SECT_SUPER (_AT(pmdval_t, 1) << 18) /* v6 */ +#define PMD_SECT_NS (_AT(pmdval_t, 1) << 19) /* v6 */ #define PMD_SECT_AF (_AT(pmdval_t, 0)) #define PMD_SECT_UNCACHED (_AT(pmdval_t, 0)) diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c index ab14b79..9baf1cb 100644 --- a/arch/arm/mm/mmu.c +++ b/arch/arm/mm/mmu.c @@ -268,6 +268,13 @@ static struct mem_type mem_types[] = { .prot_sect = PROT_SECT_DEVICE, .domain = DOMAIN_IO, }, + [MT_DEVICE_NS] = { /* Non-secure accesses from secure mode */ + .prot_pte = PROT_PTE_DEVICE | L_PTE_MT_DEV_SHARED | + L_PTE_SHARED, + .prot_l1 = PMD_TYPE_TABLE, + .prot_sect = PROT_SECT_DEVICE | PMD_SECT_S | PMD_SECT_NS, + .domain = DOMAIN_IO, + }, [MT_UNCACHED] = { .prot_pte = PROT_PTE_DEVICE, .prot_l1 = PMD_TYPE_TABLE, @@ -474,6 +481,7 @@ static void __init build_mem_type_table(void) mem_types[MT_DEVICE_NONSHARED].prot_sect |= PMD_SECT_XN; mem_types[MT_DEVICE_CACHED].prot_sect |= PMD_SECT_XN; mem_types[MT_DEVICE_WC].prot_sect |= PMD_SECT_XN; + mem_types[MT_DEVICE_NS].prot_sect |= PMD_SECT_XN; /* Also setup NX memory mapping */ mem_types[MT_MEMORY_RW].prot_sect |= PMD_SECT_XN; @@ -489,6 +497,7 @@ static void __init build_mem_type_table(void) mem_types[MT_DEVICE].prot_sect |= PMD_SECT_TEX(1); mem_types[MT_DEVICE_NONSHARED].prot_sect |= PMD_SECT_TEX(1); mem_types[MT_DEVICE_WC].prot_sect |= PMD_SECT_BUFFERABLE; + mem_types[MT_DEVICE_NS].prot_sect |= PMD_SECT_TEX(1); } else if (cpu_is_xsc3()) { /* * For Xscale3, @@ -500,6 +509,7 @@ static void __init build_mem_type_table(void) mem_types[MT_DEVICE].prot_sect |= PMD_SECT_TEX(1) | PMD_SECT_BUFFERED; mem_types[MT_DEVICE_NONSHARED].prot_sect |= PMD_SECT_TEX(2); mem_types[MT_DEVICE_WC].prot_sect |= PMD_SECT_TEX(1); + mem_types[MT_DEVICE_NS].prot_sect |= PMD_SECT_TEX(1) | PMD_SECT_BUFFERED; } else { /* * For ARMv6 and ARMv7 without TEX remapping, @@ -511,6 +521,7 @@ static void __init build_mem_type_table(void) mem_types[MT_DEVICE].prot_sect |= PMD_SECT_BUFFERED; mem_types[MT_DEVICE_NONSHARED].prot_sect |= PMD_SECT_TEX(2); mem_types[MT_DEVICE_WC].prot_sect |= PMD_SECT_TEX(1); + mem_types[MT_DEVICE_NS].prot_sect |= PMD_SECT_BUFFERED; } } else { /* @@ -856,7 +867,7 @@ static void __init create_mapping(struct map_desc *md) return; } - if ((md->type == MT_DEVICE || md->type == MT_ROM) && + if ((md->type == MT_DEVICE || md->type == MT_DEVICE_NS || md->type == MT_ROM) && md->virtual >= PAGE_OFFSET && (md->virtual < VMALLOC_START || md->virtual >= VMALLOC_END)) { printk(KERN_WARNING "BUG: mapping for 0x%08llx"