[RFC,5/9] ARM: Add L1 PTE non-secure mapping

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.

[1] See DDI0406B , Section B3 "Virtual Memory System Architecture (VMSA)",
    Subsection B3.3.1 "Translation table entry formats", paragraph
    "First-level descriptors", Table B3-1 and associated description
    of the NS bit in the "Section" table entry.

Signed-off-by: Marek Vasut <marex@denx.de>
Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 arch/arm/include/asm/io.h                   |  5 ++++-
 arch/arm/include/asm/mach/map.h             |  4 ++--
 arch/arm/include/asm/pgtable-2level-hwdef.h |  1 +
 arch/arm/mm/mmu.c                           | 13 ++++++++++++-
 4 files changed, 19 insertions(+), 4 deletions(-)

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/