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

[Ard Biesheuvel]

On 5 September 2016 at 14:24, Will Deacon <will.deacon at arm.com> wrote:
> 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?
>

Well, I do think /dev/mem is a ridiculous hack, but it is arguably an
improvement to not map a region as device if we know it is backed by
memory.

Leif already did a lot of work on dmidecode and related tools to get
rid of /dev/mem dependencies, especially because of the fact that it
used 'known' physical addresses to look for magic numbers identifying
SMBIOS tables etc. I am not sure if acpidump was on our radar yet, but
in the long term, we should be able to reduce the dependency on
/dev/mem for anything other than development use.

Would it help at all if we restricted these uses to read only? I am
not aware of any tools that require read/write access to memory in
this way.