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

[Ard Biesheuvel]

On 5 September 2016 at 15:42, Will Deacon <will.deacon at arm.com> wrote:
> On Mon, Sep 05, 2016 at 02:41:16PM +0100, Ard Biesheuvel wrote:
>> 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:
>> >> >> 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.
>
> Judging by Leif's reply, acpidump has already been fixed too.
>

Fortunately, yes.

>> 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.




>
> I'd certainly be happier if we dropped the write permission and derived
> the memory type from EFI for the NOMAP regions (it looks like ia64 tries
> to do something like this, whereas we currently return non-cacheable if
> O_SYNC is set on the fd).
>

The UEFI memory map describes the supported mapping types. I.e, most
regions are listed as

[Conventional Memory|   |  |  |  |  |  |  |   |WB|WT|WC|UC]

which means that it is legal to map it as

WB:
Normal Memory
Outer Write-back non-transient
Inner Write-back non-transient

WT:
Normal Memory
Outer Write-through non-transient
Inner Write-through non-transient

WC:
Normal Memory
Outer non-cacheable
Inner non-cacheable

UC:
Device-nGnRnE

(this is straight from the UEFI spec)

So while WB is used in the majority of cases, this is not universally
true, and the UEFI or ACPI layers may decide to use any of those for
reserved regions.

We already had a separate discussion whether it makes sense to include
the UC attribute in ACPI memory regions. The problem here is that it
is not unambiguously defined whether the attributes describe the
nature of the memory regions (i.e, the backing) or the nature of the
/contents/ of the memory regions. This is further complicated by the
fact that occupied and available regions are described in the same
way.

The bottom line is that, in general, we cannot infer from the UEFI
memory map how a region is currently mapped if it is not covered by
the linear mapping. What we /could/ do is generally disallow device
mappings for regions that can also be mapped as memory (i.e, not only
for /dev/mem) (although I think ACPI RAS stipulates Device-nGnRnE in
some cases.)