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.