Re: [PATCH] arm64: Add translation functions for /dev/mem read/write

[Goel, Sameer]

On 5/3/2017 2:18 PM, Leif Lindholm wrote:
> On Wed, May 03, 2017 at 11:07:45AM -0600, Goel, Sameer wrote:
>> On 5/3/2017 5:26 AM, Will Deacon wrote:
>>> [adding some /dev/mem fans to cc]
>>>
>>> On Tue, May 02, 2017 at 02:28:05PM -0600, Sameer Goel wrote:
>>>> Port architecture specific xlate and unxlate functions for /dev/mem
>>>> read/write. This sets up the mapping for a valid physical address if a
>>>> kernel direct mapping is not already present.
>>>>
>>>> This is a generic issue as a user space app should not be allowed to crash
>>>> the kernel.
>>>
>>>> This issue was observed when systemd tried to access performance
>>>> pointer record from the FPDT table.
>>>
>>> Why is it doing that? Is there not a way to get this via /sys?
>>
>> There is no ACPI FPDT implementation in the kernel, so the userspace
>> systemd code is getting the FPDT table contents from /sys
>> and parsing the entries. The performance pointer record is a
>> reserved address populated by UEFI and the userspace code tries to
>> access it using /dev/mem. The physical address is valid, so cannot
>> push back on the user space code.
> 
> OK, so then we need to add support for parsing this table in the
> kernel and exposing the referred-to regions in a controllable fashion.
> Maybe something that belongs under /sys/firmware/efi (adding Matt), or
> maybe something that deserves its own driver.
> 
> The only two use-cases for /dev/mem on arm64 are:
> - Implementing interfaces in the kernel takes up-front effort.
> - Being able to accidentally panic the kernel from userland.
> 
We will see this issue with any access using /dev/mem to a MEMBLOCK_NOMAP marked
memblock. The kernel crash issue has to be fixed irrespective of ACPI FPDT support.

- Sameer
> /
>     Leif
> 
>> https://github.com/systemd/systemd/blob/master/src/shared/acpi-fpdt.c
>> http://www.uefi.org/sites/default/files/resources/ACPI_6.0.pdf  5.2.23 
>>>
>>>> Ported from commit e045fb2a988a ("x86: PAT avoid aliasing in /dev/mem
>>>> read/write")
>>>>
>>>> Crash Signature:
>>>>  Unable to handle kernel paging request at virtual address ffff800008ff0000
>>>>  pgd = ffff8007de8b2200
>>>>  [ffff800008ff0000] *pgd=0000000000000000, *pud=0000000000000000
>>>>  Internal error: Oops: 96000007 [#1] SMP
>>>> ................
>>>>  CPU: 0 PID: 1 Comm: systemd Not tainted 4.10.0 #1
>>>>  task: ffff8007c0820000 task.stack: ffff8007c0900000
>>>>  PC is at __arch_copy_to_user+0xb4/0x280
>>>>  LR is at read_mem+0xc0/0x138
>>>>  pc : [<ffff0000084b3bb4>] lr : [<ffff00000869d178>]
>>>> pstate: 80000145
>>>>  sp : ffff8007c0903d40
>>>> ....................
>>>>  x3 : ffff800800000000 x2 : 0000000000000008
>>>>  x1 : ffff800008ff0000 x0 : 0000fffff6fdac00
>>>> ....................
>>>>  Call trace:
>>>>  Exception stack(0xffff8007c0903b70 to 0xffff8007c0903ca0)
>>>>  [<ffff0000084b3bb4>] __arch_copy_to_user+0xb4/0x280
>>>>  [<ffff0000082454d0>] __vfs_read+0x48/0x130
>>>>  [<ffff0000082467dc>] vfs_read+0x8c/0x148
>>>>  [<ffff000008247a34>] SyS_pread64+0x94/0xa8
>>>>  [<ffff0000080833b0>] el0_svc_naked+0x24/0x28
>>>
>>> So this certainly looks like a kernel bug, but I don't think your patch is
>>> the right way to fix it.
>>
>> I agree that the reserved regions are not meant to be accessed by the kernel as system 
>> ram. So, another option was to to return a NULL for this translation.
>>
>> Since, the same usage was working on other architectures I ported over the same code to
>> highlight the issue.
>>
>>>
>>>>  Code: a88120c7 d503201f d503201f 36180082 (f8408423)
>>>>
>>>> Signed-off-by: Sameer Goel <sgoel@xxxxxxxxxxxxxx>
>>>> Tested-by: Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>
>>>> ---
>>>>  arch/arm64/include/asm/io.h |  5 +++++
>>>>  arch/arm64/mm/ioremap.c     | 31 +++++++++++++++++++++++++++++++
>>>>  2 files changed, 36 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
>>>> index 0c00c87..c869ea4 100644
>>>> --- a/arch/arm64/include/asm/io.h
>>>> +++ b/arch/arm64/include/asm/io.h
>>>> @@ -183,6 +183,11 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>>>>  #define iowrite32be(v,p)	({ __iowmb(); __raw_writel((__force __u32)cpu_to_be32(v), p); })
>>>>  #define iowrite64be(v,p)	({ __iowmb(); __raw_writeq((__force __u64)cpu_to_be64(v), p); })
>>>>  
>>>> +extern void *xlate_dev_mem_ptr(phys_addr_t phys);
>>>> +extern void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr);
>>>> +
>>>> +#define xlate_dev_mem_ptr xlate_dev_mem_ptr
>>>> +#define unxlate_dev_mem_ptr unxlate_dev_mem_ptr
>>>>  #include <asm-generic/io.h>
>>>>  
>>>>  /*
>>>> diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c
>>>> index c4c8cd4..ba7e63b 100644
>>>> --- a/arch/arm64/mm/ioremap.c
>>>> +++ b/arch/arm64/mm/ioremap.c
>>>> @@ -24,6 +24,7 @@
>>>>  #include <linux/mm.h>
>>>>  #include <linux/vmalloc.h>
>>>>  #include <linux/io.h>
>>>> +#include <linux/memblock.h>
>>>>  
>>>>  #include <asm/fixmap.h>
>>>>  #include <asm/tlbflush.h>
>>>> @@ -105,6 +106,36 @@ void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size)
>>>>  EXPORT_SYMBOL(ioremap_cache);
>>>>  
>>>>  /*
>>>> + * Convert a physical pointer to a virtual kernel pointer for /dev/mem
>>>> + * access
>>>> + */
>>>> +void *xlate_dev_mem_ptr(phys_addr_t phys)
>>>> +{
>>>> +	unsigned long start  = phys &  PAGE_MASK;
>>>> +	unsigned long offset = phys & ~PAGE_MASK;
>>>> +	void *vaddr;
>>>> +
>>>> +	/* If page is RAM, we can use __va. Otherwise ioremap and unmap. */
>>>> +	if (page_is_ram(start >> PAGE_SHIFT) && memblock_is_memory(phys))
>>>> +		return __va(phys);
>>>> +
>>>> +	vaddr = ioremap_cache(start, PAGE_SIZE);
>>>
>>> Blindly using ioremap like this looks unsafe, since we could accidentally
>>> set conflict with the attributes of a mapping used by something else (e.g.
>>> firmware running on another CPU).
>>>
>>> I'd like to understand more about the crash, so we can see work out how to
>>> fix this properly.
>>>
>> This does opens up access to any valid physical address. In the short term we 
>> can block this crash by return NULL from this function if the memblock is MEMBLOCK_NOMAP.
>>
>> Eventually we might need to add another memory type to make sure that it can be mapped.
>> I have not though about the exact design here.
>>
>> Thanks,
>> Sameer
>>  
>>> Will
>>>
>>
>> -- 
>>  Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.