[PATCH] arm64: mm: phys_mem_access_prot() reports non-kernel memory as device memory

[Will Deacon]

On Mon, Sep 05, 2016 at 02:10:54PM +0100, Ard Biesheuvel wrote:
> On 5 September 2016 at 14:03, James Morse <james.morse at arm.com> wrote:
> > On 05/09/16 11:35, Will Deacon wrote:
> >> On Mon, Sep 05, 2016 at 09:43:03AM +0100, James Morse wrote:
> >>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >>> index 4989948d1feb..96a2f327fd2c 100644
> >>> --- a/arch/arm64/mm/mmu.c
> >>> +++ b/arch/arm64/mm/mmu.c
> >>> @@ -63,7 +63,7 @@ static pud_t bm_pud[PTRS_PER_PUD] __page_aligned_bss __maybe_unused;
> >>>  pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
> >>>                            unsigned long size, pgprot_t vma_prot)
> >>>  {
> >>> -    if (!pfn_valid(pfn))
> >>> +    if (!memblock_is_memory(pfn << PAGE_SHIFT))
> >
> >> Hmm, this looks pretty weird to me. pfn_valid currently calls
> >> memblock_is_map_memory, so now we have this slightly paradoxical situation
> >> of having some memory marked NOMAP but wanting to map it to userspace.
> >
> > Yup, crazy as it looks, that is what is happening.
> >
> 
> Indeed. Formerly, we would remove this from memblock entirely,
> resulting in the same situation, i.e., that the region is omitted from
> the linear mapping. However, by doing that, we also lose the
> annotation that the region was memory to begin with, resulting in the
> ACPI layer using ioremap() rather than ioremap_cache() or memremap()
> to map it.
> 
> So the whole point of introducing MEMBLOCK_NOMAP was to allow memory
> to be kept in the memblock array, but with an annotation that it
> should not be covered by the linear mapping.
> 
> >> So our abstractions don't seem to align with reality. Why are the ACPI
> >> tables getting marked as NOMAP to being with?
> >
> > EFI adds any region we can map as normal memory to memblock.memory, it also adds
> > anything it considers reserved to the memblock.nomap, e.g. the ACPI tables. This
> > causes these regions be left out of the linear map.
> > (I don't know why exactly, but assume its so that the attributes and permissions
> > can be tweaked easily)
> >
> 
> To prevent mismatched attributes between the linear mapping and
> whatever mapping the firmware and/or ACPI are using. Also, going
> through the trouble of mapping runtime services executable code with
> R-X permissions and then having a writable alias is not great in terms
> of security.

Ok, but with this patch we potentially have both of those problems
(mismatched attributes and RWX permission) with the userspace mapping :/

Is it worth trying to avoid any of this, or do we just throw our hands
in the air and give up when /dev/mem is being used?

Will