ARM64: Kernel managed pages are only flushed

PATCH New Bharat Bhushan 4 participants 6 messages Mar 5, 2014 – Mar 6, 2014 arm64

lore

Share & tools

lore

Bharat Bhushan

author

2014-03-05 11:25 UTC #0 lore raw

PATCH v2 1/1 ARM64: Kernel managed pages are only flushed

Kernel can only access pages which maps to managed memory. So flush only valid kernel pages.

I observed kernel crash direct assigning a device using VFIO and found that it was caused because of accessing invalid page

Signed-off-by Bharat Bhushan <[email protected]>

---
v1->v2
Getting pfn usin pte_pfn() in pfn_valid.

 arch/arm64/mm/flush.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

arch/arm64/mm/flush.c +12 -1

index e4193e3..319826a 100644
@@ -72,7 +72,18 @@ void copy_to_user_page(struct vm_area_struct *vma, struct page *page,
 
 void __sync_icache_dcache(pte_t pte, unsigned long addr)
 {
-	struct page *page = pte_page(pte);
+	struct page *page;
+
+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+	/*
+	 * We can only access pages that the kernel maps
+	 * as memory. Bail out for unmapped ones.
+	 */
+	if (!pfn_valid(pte_pfn(pte)))
+		return;
+
+#endif
+	page = pte_page(pte);
 
 	/* no flushing needed for anonymous pages */
 	if (!page_mapping(page))

Signature · 1 line

1.7.0.4

Will Deacon

2014-03-05 16:12 UTC #1 lore raw

PATCH v2 1/1 ARM64: Kernel managed pages are only flushed

Quoted 25 lines · Bharat Bhushan

Kernel can only access pages which maps to managed memory. So flush only valid kernel pages.
I observed kernel crash direct assigning a device using VFIO and found that it was caused because of accessing invalid page
Signed-off-by Bharat Bhushan <[email protected]>
---
v1->v2
Getting pfn usin pte_pfn() in pfn_valid.
 arch/arm64/mm/flush.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)
arch/arm64/mm/flush.c +2 -1
index e4193e3..319826a 100644
@@ -72,7 +72,18 @@ void copy_to_user_page(struct vm_area_struct *vma, struct page *page,
 
 void __sync_icache_dcache(pte_t pte, unsigned long addr)
 {
-	struct page *page = pte_page(pte);
+	struct page *page;
+

arch/arm64/mm/flush.c void copy_to_user_page(struct vm_area_struct *vma, struct page *page, · line 86

Quoted 10 lines

arch/arm64/mm/flush.c +10 -0

+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+	/*
+	 * We can only access pages that the kernel maps
+	 * as memory. Bail out for unmapped ones.
+	 */
+	if (!pfn_valid(pte_pfn(pte)))
+		return;
+
+#endif
+	page = pte_page(pte);

How do you get into this function without a valid, userspace, executable pte?

I suspect you've got changes elsewhere and are calling this function in a context where it's not supposed to be called.

Will

Bharat.Bhushan at freescale.com

2014-03-05 16:27 UTC #2 lore raw

PATCH v2 1/1 ARM64: Kernel managed pages are only flushed

Quoted 48 lines

-----Original Message-----
From: Will Deacon [mailto:will.deacon at arm.com]
Sent: Wednesday, March 05, 2014 9:43 PM
To: Bhushan Bharat-R65777
Cc: Catalin Marinas; linux-arm-kernel at lists.infradead.org; Bhushan Bharat-R65777
Subject: Re: [PATCH v2] ARM64: Kernel managed pages are only flushed
Quoted 25 lines · Bharat Bhushan
Kernel can only access pages which maps to managed memory. So flush only valid kernel pages.
I observed kernel crash direct assigning a device using VFIO and found that it was caused because of accessing invalid page
Signed-off-by Bharat Bhushan <[email protected]>
---
v1->v2
Getting pfn usin pte_pfn() in pfn_valid.
 arch/arm64/mm/flush.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)
arch/arm64/mm/flush.c index renamed from arch/arm64/mm/flush.c +0 -0
e4193e3..319826a 100644
arch/arm64/mm/flush.c +2 -1
@@ -72,7 +72,18 @@ void copy_to_user_page(struct vm_area_struct *vma,
 struct page *page,
 
 void __sync_icache_dcache(pte_t pte, unsigned long addr)  {
-	struct page *page = pte_page(pte);
+	struct page *page;
+
arch/arm64/mm/flush.c void copy_to_user_page(struct vm_area_struct *vma, · line 86
Quoted 10 lines
arch/arm64/mm/flush.c +10 -0
+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+	/*
+	 * We can only access pages that the kernel maps
+	 * as memory. Bail out for unmapped ones.
+	 */
+	if (!pfn_valid(pte_pfn(pte)))
+		return;
+
+#endif
+	page = pte_page(pte);
How do you get into this function without a valid, userspace, executable pte?

Quoted 2 lines

I suspect you've got changes elsewhere and are calling this function in a context where it's not supposed to be called.

Below I will describe the context in which this function is called:

When we direct assign a bus device (we have a different freescale specific bus device but we can take PCI device for discussion as this logic applies equally for PCI device I think) to user space using VFIO. Then userspace needs to mmap(PCI_BARx_offset: this PCI bar offset in not a kernel visible memory). Then VFIO-kernel mmap() ioctl code calls remap_pfn_range() for mapping the requested address. While remap_pfn_range() internally calls this function.

Thanks
-Bharat

Quoted 3 lines

Will

Laura Abbott

2014-03-05 20:03 UTC #3 lore raw

PATCH v2 1/1 ARM64: Kernel managed pages are only flushed

Quoted 48 lines · Bharat.Bhushan at freescale.com

Quoted 48 lines
-----Original Message-----
From: Will Deacon [mailto:will.deacon at arm.com]
Sent: Wednesday, March 05, 2014 9:43 PM
To: Bhushan Bharat-R65777
Cc: Catalin Marinas; linux-arm-kernel at lists.infradead.org; Bhushan Bharat-R65777
Subject: Re: [PATCH v2] ARM64: Kernel managed pages are only flushed
Quoted 25 lines · Bharat Bhushan
Kernel can only access pages which maps to managed memory. So flush only valid kernel pages.
I observed kernel crash direct assigning a device using VFIO and found that it was caused because of accessing invalid page
Signed-off-by Bharat Bhushan <[email protected]>
---
v1->v2
Getting pfn usin pte_pfn() in pfn_valid.
  arch/arm64/mm/flush.c |   13 ++++++++++++-
  1 files changed, 12 insertions(+), 1 deletions(-)
arch/arm64/mm/flush.c index renamed from arch/arm64/mm/flush.c +0 -0
e4193e3..319826a 100644
arch/arm64/mm/flush.c +2 -1
@@ -72,7 +72,18 @@ void copy_to_user_page(struct vm_area_struct *vma,
 struct page *page,
 
  void __sync_icache_dcache(pte_t pte, unsigned long addr)  {
-	struct page *page = pte_page(pte);
+	struct page *page;
+
arch/arm64/mm/flush.c void copy_to_user_page(struct vm_area_struct *vma, · line 86
Quoted 10 lines
arch/arm64/mm/flush.c +10 -0
+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+	/*
+	 * We can only access pages that the kernel maps
+	 * as memory. Bail out for unmapped ones.
+	 */
+	if (!pfn_valid(pte_pfn(pte)))
+		return;
+
+#endif
+	page = pte_page(pte);
How do you get into this function without a valid, userspace, executable pte?

Quoted 7 lines

Quoted 2 lines
I suspect you've got changes elsewhere and are calling this function in a context where it's not supposed to be called.
Below I will describe the context in which this function is called:
When we direct assign a bus device (we have a different freescale specific bus device but we can take PCI device for discussion as this logic

applies equally

Quoted 2 lines

for PCI device I think) to user space using VFIO. Then userspace needs to mmap(PCI_BARx_offset: this PCI bar offset in not a kernel visible

memory).

Quoted 3 lines

Then VFIO-kernel mmap() ioctl code calls remap_pfn_range() for mapping the requested address. While remap_pfn_range() internally calls this function.

As someone who likes calling functions in context where they aren't supposed to be called, I took a look a this because I was curious.

I can confirm the same problem trying to mmap arbitrary io address space with remap_pfn_range. We should only be hitting this if the pte is marked as exec per set_pte_at. With my test case, even mmaping with only PROT_READ and PROT_WRITE was setting PROT_EXEC as well which was triggering the bug. This seems to be because READ_IMPLIES_EXEC personality was set which was derived from

#define elf_read_implies_exec(ex,stk) (stk != EXSTACK_DISABLE_X)

and none of the binaries I'm generating seem to be setting the stack execute bit either way (all are EXECSTACK_DEFAULT).

It's not obvious what the best solution is here.

Thanks,
Laura

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Bharat.Bhushan at freescale.com

2014-03-06 03:38 UTC #4 lore raw

PATCH v2 1/1 ARM64: Kernel managed pages are only flushed

Quoted 71 lines

-----Original Message-----
From: Laura Abbott [mailto:lauraa at codeaurora.org]
Sent: Thursday, March 06, 2014 1:34 AM
To: Bhushan Bharat-R65777; Will Deacon
Cc: Wood Scott-B07421; Catalin Marinas; Yoder Stuart-B08248; linux-arm- kernel at lists.infradead.org
Subject: Re: [PATCH v2] ARM64: Kernel managed pages are only flushed
Quoted 49 lines · Bharat.Bhushan at freescale.com
Quoted 49 lines
-----Original Message-----
From: Will Deacon [mailto:will.deacon at arm.com]
Sent: Wednesday, March 05, 2014 9:43 PM
To: Bhushan Bharat-R65777
Cc: Catalin Marinas; linux-arm-kernel at lists.infradead.org; Bhushan Bharat-R65777
Subject: Re: [PATCH v2] ARM64: Kernel managed pages are only flushed
Quoted 25 lines · Bharat Bhushan
Kernel can only access pages which maps to managed memory. So flush only valid kernel pages.
I observed kernel crash direct assigning a device using VFIO and found that it was caused because of accessing invalid page
Signed-off-by Bharat Bhushan <[email protected]>
---
v1->v2
Getting pfn usin pte_pfn() in pfn_valid.
  arch/arm64/mm/flush.c |   13 ++++++++++++-
  1 files changed, 12 insertions(+), 1 deletions(-)
arch/arm64/mm/flush.c index renamed from arch/arm64/mm/flush.c +0 -0
e4193e3..319826a 100644
arch/arm64/mm/flush.c +2 -1
@@ -72,7 +72,18 @@ void copy_to_user_page(struct vm_area_struct
 *vma, struct page *page,
 
  void __sync_icache_dcache(pte_t pte, unsigned long addr)  {
-	struct page *page = pte_page(pte);
+	struct page *page;
+
arch/arm64/mm/flush.c void copy_to_user_page(struct vm_area_struct · line 86
Quoted 10 lines
arch/arm64/mm/flush.c +10 -0
+#ifdef CONFIG_HAVE_ARCH_PFN_VALID
+	/*
+	 * We can only access pages that the kernel maps
+	 * as memory. Bail out for unmapped ones.
+	 */
+	if (!pfn_valid(pte_pfn(pte)))
+		return;
+
+#endif
+	page = pte_page(pte);
How do you get into this function without a valid, userspace, executable pte?
Quoted 8 lines
Quoted 2 lines
I suspect you've got changes elsewhere and are calling this function in a context where it's not supposed to be called.
Below I will describe the context in which this function is called:
When we direct assign a bus device (we have a different freescale specific bus
device but we can take PCI device for discussion as this logic applies
equally > for PCI device I think) to user space using VFIO. Then userspace needs to > mmap(PCI_BARx_offset: this PCI bar offset in not a kernel visible memory).

Quoted 7 lines

Quoted 4 lines
Then VFIO-kernel mmap() ioctl code calls remap_pfn_range() for mapping the
requested address. While remap_pfn_range() internally calls this function.
As someone who likes calling functions in context where they aren't supposed to be called, I took a look a this because I was curious.

Are we saying that remap_pfn_range() should not be called in such case (described earlier the case of direct assigning PCI device to user space using VFIO) ? But x86/powerpc calls this function only.

Quoted 11 lines

I can confirm the same problem trying to mmap arbitrary io address space with remap_pfn_range. We should only be hitting this if the pte is marked as exec per set_pte_at. With my test case, even mmaping with only PROT_READ and PROT_WRITE was setting PROT_EXEC as well which was triggering the bug. This seems to be because READ_IMPLIES_EXEC personality was set which was derived from
#define elf_read_implies_exec(ex,stk) (stk != EXSTACK_DISABLE_X)
and none of the binaries I'm generating seem to be setting the stack execute bit either way (all are EXECSTACK_DEFAULT).

Yes I agree that even if we set PROT_READ and PROT_WRITE but it internally end up setting PROT_EXEC, so we enter in flow. But I see this as a second issue. I am not sure but theoretically it can still happen that we set PROT_EXEC for anonymous page.

So either __sync_icache_dcache() should check that it does not access anonymous struct page (which this patch is doing) or __sync_icache_dcache() should not be called for anonymous page. Maybe something like this:

arch/arm64/include/asm/pgtable.h +1 -1

index f0bebc5..9493f3e 100644
@@ -167,7 +167,7 @@ static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
                              pte_t *ptep, pte_t pte)
 {
        if (pte_valid_user(pte)) {
-               if (pte_exec(pte))
+               if (pte_exec(pte) && pfn_valid(pte_pfn(pte)))
                        __sync_icache_dcache(pte, addr);
                if (!pte_dirty(pte))
                        pte = pte_wrprotect(pte);

Please suggest if some other solution.

Thanks
-Bharat

Quoted 10 lines

It's not obvious what the best solution is here.
Thanks,
Laura
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Will Deacon

2014-03-06 16:18 UTC #5 lore raw

PATCH v2 1/1 ARM64: Kernel managed pages are only flushed

In reply to Laura Abbott

Hi Laura,

Quoted 17 lines · Laura Abbott

Quoted 6 lines · Bharat.Bhushan at freescale.com
Quoted 6 lines · Bharat Bhushan
Quoted 3 lines
Kernel can only access pages which maps to managed memory. So flush only valid kernel pages.
How do you get into this function without a valid, userspace, executable pte?
Quoted 7 lines
Quoted 2 lines
I suspect you've got changes elsewhere and are calling this function in a context where it's not supposed to be called.
Below I will describe the context in which this function is called:
When we direct assign a bus device (we have a different freescale specific bus device but we can take PCI device for discussion as this logic
applies equally
Quoted 2 lines
for PCI device I think) to user space using VFIO. Then userspace needs to mmap(PCI_BARx_offset: this PCI bar offset in not a kernel visible

Quoted 7 lines

memory).
Quoted 3 lines
Then VFIO-kernel mmap() ioctl code calls remap_pfn_range() for mapping the requested address. While remap_pfn_range() internally calls this function.
As someone who likes calling functions in context where they aren't supposed to be called, I took a look a this because I was curious.

Somebody should hide your keyboard. Stephen?

Quoted 7 lines

I can confirm the same problem trying to mmap arbitrary io address space with remap_pfn_range. We should only be hitting this if the pte is marked as exec per set_pte_at. With my test case, even mmaping with only PROT_READ and PROT_WRITE was setting PROT_EXEC as well which was triggering the bug. This seems to be because READ_IMPLIES_EXEC personality was set which was derived from

Quoted 6 lines

#define elf_read_implies_exec(ex,stk) (stk != EXSTACK_DISABLE_X)
and none of the binaries I'm generating seem to be setting the stack execute bit either way (all are EXECSTACK_DEFAULT).
It's not obvious what the best solution is here.

It would be nice if something like phys_mem_access_prot was used by the callers, since this is used by the /dev/mem driver to make sure that the pgprot is sane for the underlying pfn. In the absence of that, I guess we could add the pfn_valid check (we have it already on arch/arm/) but if that means we end up with executable devices, we're still entering a world of looks-like-my-instruction-fetcher-just-acked-an-irq style pain.

Will