[PATCH v2] arm64: kernel: restrict /dev/mem read() calls to linear region

[PATCH v2] arm64: kernel: restrict /dev/mem read() calls to linear region

[Ard Biesheuvel]

When running lscpu on an AArch64 system that has SMBIOS version 2.0
tables, it will segfault in the following way:

  Unable to handle kernel paging request at virtual address ffff8000bfff0000
  pgd = ffff8000f9615000
  [ffff8000bfff0000] *pgd=0000000000000000
  Internal error: Oops: 96000007 [#1] PREEMPT SMP
  Modules linked in:
  CPU: 0 PID: 1284 Comm: lscpu Not tainted 4.11.0-rc3+ #103
  Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
  task: ffff8000fa78e800 task.stack: ffff8000f9780000
  PC is at __arch_copy_to_user+0x90/0x220
  LR is at read_mem+0xcc/0x140

This is caused by the fact that lspci issues a read() on /dev/mem at the
offset where it expects to find the SMBIOS structure array. However, this
region is classified as EFI_RUNTIME_SERVICE_DATA (as per the UEFI spec),
and so it is omitted from the linear mapping.

So let's restrict /dev/mem read/write access to those areas that are
covered by the linear region.

Reported-by: Alexander Graf <agraf@suse.de>
Fixes: 4dffbfc48d65 ("arm64/efi: mark UEFI reserved regions as MEMBLOCK_NOMAP")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
v2: check whether the entire region is covered by the same memblock that has
    the MEMBLOCK_NOMAP attribute cleared

 arch/arm64/mm/mmap.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
index 7b0d55756eb1..adc208c2ae9c 100644
--- a/arch/arm64/mm/mmap.c
+++ b/arch/arm64/mm/mmap.c
@@ -18,6 +18,7 @@
 
 #include <linux/elf.h>
 #include <linux/fs.h>
+#include <linux/memblock.h>
 #include <linux/mm.h>
 #include <linux/mman.h>
 #include <linux/export.h>
@@ -103,12 +104,18 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
  */
 int valid_phys_addr_range(phys_addr_t addr, size_t size)
 {
-	if (addr < PHYS_OFFSET)
-		return 0;
-	if (addr + size > __pa(high_memory - 1) + 1)
-		return 0;
-
-	return 1;
+	/*
+	 * Check whether addr is covered by a memory region without the
+	 * MEMBLOCK_NOMAP attribute, and whether that region covers the
+	 * entire range. In theory, this could lead to false negatives
+	 * if the range is covered by distinct but adjacent memory regions
+	 * that only differ in other attributes. However, few of such
+	 * attributes have been defined, and it is debatable whether it
+	 * follows that /dev/mem read() calls should be able traverse
+	 * such boundaries.
+	 */
+	return memblock_is_region_memory(addr, size) &&
+	       memblock_is_map_memory(addr);
 }
 
 /*
-- 
2.9.3

[PATCH v2] arm64: kernel: restrict /dev/mem read() calls to linear region

[Leif Lindholm]

On Fri, May 19, 2017 at 04:42:00PM +0100, Ard Biesheuvel wrote:
> When running lscpu on an AArch64 system that has SMBIOS version 2.0
> tables, it will segfault in the following way:
> 
>   Unable to handle kernel paging request at virtual address ffff8000bfff0000
>   pgd = ffff8000f9615000
>   [ffff8000bfff0000] *pgd=0000000000000000
>   Internal error: Oops: 96000007 [#1] PREEMPT SMP
>   Modules linked in:
>   CPU: 0 PID: 1284 Comm: lscpu Not tainted 4.11.0-rc3+ #103
>   Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
>   task: ffff8000fa78e800 task.stack: ffff8000f9780000
>   PC is at __arch_copy_to_user+0x90/0x220
>   LR is at read_mem+0xcc/0x140
> 
> This is caused by the fact that lspci issues a read() on /dev/mem at the
> offset where it expects to find the SMBIOS structure array. However, this
> region is classified as EFI_RUNTIME_SERVICE_DATA (as per the UEFI spec),
> and so it is omitted from the linear mapping.
> 
> So let's restrict /dev/mem read/write access to those areas that are
> covered by the linear region.

So, I'm still of the opinion that /dev/mem simply should not be made
available on systems where people care about accidentally hard-locking
their systems from userland.

To that extent, this workaround takes the pressure off people to
configure their kernels properly.

On the other hand, it probably removes 90% of the risk cases.

I guess the solution depends on whether people think the remaining 10%
matter.

/
    Leif

> Reported-by: Alexander Graf <agraf@suse.de>
> Fixes: 4dffbfc48d65 ("arm64/efi: mark UEFI reserved regions as MEMBLOCK_NOMAP")
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: check whether the entire region is covered by the same memblock that has
>     the MEMBLOCK_NOMAP attribute cleared
> 
>  arch/arm64/mm/mmap.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
> index 7b0d55756eb1..adc208c2ae9c 100644
> --- a/arch/arm64/mm/mmap.c
> +++ b/arch/arm64/mm/mmap.c
> @@ -18,6 +18,7 @@
>  
>  #include <linux/elf.h>
>  #include <linux/fs.h>
> +#include <linux/memblock.h>
>  #include <linux/mm.h>
>  #include <linux/mman.h>
>  #include <linux/export.h>
> @@ -103,12 +104,18 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
>   */
>  int valid_phys_addr_range(phys_addr_t addr, size_t size)
>  {
> -	if (addr < PHYS_OFFSET)
> -		return 0;
> -	if (addr + size > __pa(high_memory - 1) + 1)
> -		return 0;
> -
> -	return 1;
> +	/*
> +	 * Check whether addr is covered by a memory region without the
> +	 * MEMBLOCK_NOMAP attribute, and whether that region covers the
> +	 * entire range. In theory, this could lead to false negatives
> +	 * if the range is covered by distinct but adjacent memory regions
> +	 * that only differ in other attributes. However, few of such
> +	 * attributes have been defined, and it is debatable whether it
> +	 * follows that /dev/mem read() calls should be able traverse
> +	 * such boundaries.
> +	 */
> +	return memblock_is_region_memory(addr, size) &&
> +	       memblock_is_map_memory(addr);
>  }
>  
>  /*
> -- 
> 2.9.3
> 

[PATCH v2] arm64: kernel: restrict /dev/mem read() calls to linear region

[Ard Biesheuvel]

On 22 May 2017 at 10:41, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, May 19, 2017 at 04:42:00PM +0100, Ard Biesheuvel wrote:
>> When running lscpu on an AArch64 system that has SMBIOS version 2.0
>> tables, it will segfault in the following way:
>>
>>   Unable to handle kernel paging request at virtual address ffff8000bfff0000
>>   pgd = ffff8000f9615000
>>   [ffff8000bfff0000] *pgd=0000000000000000
>>   Internal error: Oops: 96000007 [#1] PREEMPT SMP
>>   Modules linked in:
>>   CPU: 0 PID: 1284 Comm: lscpu Not tainted 4.11.0-rc3+ #103
>>   Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
>>   task: ffff8000fa78e800 task.stack: ffff8000f9780000
>>   PC is at __arch_copy_to_user+0x90/0x220
>>   LR is at read_mem+0xcc/0x140
>>
>> This is caused by the fact that lspci issues a read() on /dev/mem at the
>> offset where it expects to find the SMBIOS structure array. However, this
>> region is classified as EFI_RUNTIME_SERVICE_DATA (as per the UEFI spec),
>> and so it is omitted from the linear mapping.
>>
>> So let's restrict /dev/mem read/write access to those areas that are
>> covered by the linear region.
>
> So, I'm still of the opinion that /dev/mem simply should not be made
> available on systems where people care about accidentally hard-locking
> their systems from userland.
>
> To that extent, this workaround takes the pressure off people to
> configure their kernels properly.
>
> On the other hand, it probably removes 90% of the risk cases.
>

Sadly, no. This change only fixes a kernel bug where we access a
region using read()/write() that is known to be memory, but may have
been left unmapped. It does not affect /dev/mem uses involving mmap()
at all.

> I guess the solution depends on whether people think the remaining 10%
> matter.
>

I think there are two different aspects that we need to distinguish:
- on ARM, you cannot use /dev/mem to go looking for magic numbers in
memory to locate certain data structures left by the firmware,
- on ARM, you should [must] not use /dev/mem to map regions that are
already mapped with different attributes.

This fix is in neither category, it just fails read()s on /dev/mem
gracefully if they attempt to access NOMAP memory.

So the only moderately sane use case for /dev/mem on ARM is to do
cached accesses to DRAM regions that are covered by the linear
mapping. Interestingly, these are exactly the ones RESTRICT_DEVMEM
usually disallows IIRC, given that you may be poking into kernel
internals.

My vote would be to deprecate other /dev/mem uses on arm64, so that we
can start getting a handle on things before the cat is too far out of
the bag. (and you can't 'break' a x86 userspace this way on arm64 if
it was simply recompiled but pokes memory for legacy BIOS strings etc)

[PATCH v2] arm64: kernel: restrict /dev/mem read() calls to linear region

[Leif Lindholm]

On Tue, May 23, 2017 at 03:51:11AM -0700, Ard Biesheuvel wrote:
> > So, I'm still of the opinion that /dev/mem simply should not be made
> > available on systems where people care about accidentally hard-locking
> > their systems from userland.
> >
> > To that extent, this workaround takes the pressure off people to
> > configure their kernels properly.
> >
> > On the other hand, it probably removes 90% of the risk cases.
> 
> Sadly, no. This change only fixes a kernel bug where we access a
> region using read()/write() that is known to be memory, but may have
> been left unmapped. It does not affect /dev/mem uses involving mmap()
> at all.

Ah. Well, in that case I don't have much of an objection.

> > I guess the solution depends on whether people think the remaining 10%
> > matter.
> 
> I think there are two different aspects that we need to distinguish:
> - on ARM, you cannot use /dev/mem to go looking for magic numbers in
> memory to locate certain data structures left by the firmware,
> - on ARM, you should [must] not use /dev/mem to map regions that are
> already mapped with different attributes.

And don't leave out the whopper:
- on ARM, running userland applications written to use /dev/mem to do
  specific things on x86 is unlikely to do what you expected it to do,
  and is likely to crash your system.

Being architecturally safe does not mean your system is not
vulnerable.

> This fix is in neither category, it just fails read()s on /dev/mem
> gracefully if they attempt to access NOMAP memory.

Understood.

> So the only moderately sane use case for /dev/mem on ARM is to do
> cached accesses to DRAM regions that are covered by the linear
> mapping.

Aside from "writing proper kernel interfaces takes effort", is there a
valid reason to do so?

Even if we did restrict it down to cached known DRAM regions, we would
still need to turn it read-only. Or a badly written program (I've
heard a couple exist) can start trampling whatever.

If people need a generic way to easily give access to certain defined
memory regions, let's write a driver that chomps kernel command line
or DT and provides simple mmap access to those specific regions.

If people want arbitrary userspace access to any region in memory at
any given time, why did we waste the gates and energy of implementing
an MMU and managing permissions?

> Interestingly, these are exactly the ones RESTRICT_DEVMEM
> usually disallows IIRC, given that you may be poking into kernel
> internals.

Yes, well, given its history as being PROMISC_DEVMEM renamed and
flipped logically, I doubt that't the only inconsistency...

> My vote would be to deprecate other /dev/mem uses on arm64, so that we
> can start getting a handle on things before the cat is too far out of
> the bag. (and you can't 'break' a x86 userspace this way on arm64 if
> it was simply recompiled but pokes memory for legacy BIOS strings etc)

To no one's surprise, my vote is to deprecate all uses of /dev/mem on
arm64. 

/
    Leif

[PATCH v2] arm64: kernel: restrict /dev/mem read() calls to linear region

[Will Deacon]

Hi Ard,

On Fri, May 19, 2017 at 04:42:00PM +0100, Ard Biesheuvel wrote:
> When running lscpu on an AArch64 system that has SMBIOS version 2.0
> tables, it will segfault in the following way:
> 
>   Unable to handle kernel paging request at virtual address ffff8000bfff0000
>   pgd = ffff8000f9615000
>   [ffff8000bfff0000] *pgd=0000000000000000
>   Internal error: Oops: 96000007 [#1] PREEMPT SMP
>   Modules linked in:
>   CPU: 0 PID: 1284 Comm: lscpu Not tainted 4.11.0-rc3+ #103
>   Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
>   task: ffff8000fa78e800 task.stack: ffff8000f9780000
>   PC is at __arch_copy_to_user+0x90/0x220
>   LR is at read_mem+0xcc/0x140
> 
> This is caused by the fact that lspci issues a read() on /dev/mem at the
> offset where it expects to find the SMBIOS structure array. However, this
> region is classified as EFI_RUNTIME_SERVICE_DATA (as per the UEFI spec),
> and so it is omitted from the linear mapping.
> 
> So let's restrict /dev/mem read/write access to those areas that are
> covered by the linear region.
> 
> Reported-by: Alexander Graf <agraf@suse.de>
> Fixes: 4dffbfc48d65 ("arm64/efi: mark UEFI reserved regions as MEMBLOCK_NOMAP")
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: check whether the entire region is covered by the same memblock that has
>     the MEMBLOCK_NOMAP attribute cleared
> 
>  arch/arm64/mm/mmap.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)

Thanks. I'll stick this into -next and see how we do.

Will