Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1728335 > unrolled thread

[PATCH v6 00/11] Add support for eXclusive Page Frame Ownership

Back to article view | Back to linux.kernel

Contents

  [PATCH v6 00/11] Add support for eXclusive Page Frame Ownership Tycho Andersen <tycho@docker.com> - 2017-09-07 19:40 +0200
    [PATCH v6 05/11] arm64/mm: Add support for XPFO Tycho Andersen <tycho@docker.com> - 2017-09-07 19:40 +0200
      Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO Christoph Hellwig <hch@infradead.org> - 2017-09-08 10:00 +0200
        Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO Tycho Andersen <tycho@docker.com> - 2017-09-08 19:30 +0200
          Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO Julien Grall <julien.grall@arm.com> - 2017-09-14 12:50 +0200
            Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO Juergen Gross <jgross@suse.com> - 2017-09-14 13:40 +0200
      Re: [kernel-hardening] [PATCH v6 05/11] arm64/mm: Add support for  XPFO Mark Rutland <mark.rutland@arm.com> - 2017-09-14 20:30 +0200
        Re: [kernel-hardening] [PATCH v6 05/11] arm64/mm: Add support for  XPFO Tycho Andersen <tycho@docker.com> - 2017-09-18 23:30 +0200
    [PATCH v6 01/11] mm: add MAP_HUGETLB support to vm_mmap Tycho Andersen <tycho@docker.com> - 2017-09-07 19:40 +0200
      Re: [PATCH v6 01/11] mm: add MAP_HUGETLB support to vm_mmap Christoph Hellwig <hch@infradead.org> - 2017-09-08 09:50 +0200
    [PATCH v6 02/11] x86: always set IF before oopsing from page fault Tycho Andersen <tycho@docker.com> - 2017-09-07 19:40 +0200
    Re: [PATCH v6 00/11] Add support for eXclusive Page Frame Ownership Yisheng Xie <xieyisheng1@huawei.com> - 2017-09-11 12:40 +0200
      Re: [PATCH v6 00/11] Add support for eXclusive Page Frame Ownership Tycho Andersen <tycho@docker.com> - 2017-09-11 17:10 +0200
        Re: [PATCH v6 00/11] Add support for eXclusive Page Frame Ownership Yisheng Xie <xieyisheng1@huawei.com> - 2017-09-12 09:10 +0200
          Re: [PATCH v6 00/11] Add support for eXclusive Page Frame Ownership Juerg Haefliger <juerg.haefliger@canonical.com> - 2017-09-12 09:50 +0200
            Re: [PATCH v6 00/11] Add support for eXclusive Page Frame Ownership Yisheng Xie <xieyisheng1@huawei.com> - 2017-09-12 10:20 +0200

#1728335 — [PATCH v6 00/11] Add support for eXclusive Page Frame Ownership

Hi all,

Here is v6 of the XPFO set; see v5 discussion here:
https://lkml.org/lkml/2017/8/9/803

Changelogs are in the individual patch notes, but the highlights are:
* add primitives for ensuring memory areas are mapped (although these are quite
  ugly, using stack allocation; I'm open to better suggestions)
* instead of not flushing caches, re-map pages using the above
* TLB flushing is much more correct (i.e. we're always flushing everything
  everywhere). I suspect we may be able to back this off in some cases, but I'm
  still trying to collect performance numbers to prove this is worth doing.

I have no TODOs left for this set myself, other than fixing whatever review
feedback people have. Thoughts and testing welcome!

Cheers,

Tycho

Juerg Haefliger (6):
  mm, x86: Add support for eXclusive Page Frame Ownership (XPFO)
  swiotlb: Map the buffer if it was unmapped by XPFO
  arm64/mm: Add support for XPFO
  arm64/mm, xpfo: temporarily map dcache regions
  arm64/mm: Add support for XPFO to swiotlb
  lkdtm: Add test for XPFO

Tycho Andersen (5):
  mm: add MAP_HUGETLB support to vm_mmap
  x86: always set IF before oopsing from page fault
  xpfo: add primitives for mapping underlying memory
  arm64/mm: disable section/contiguous mappings if XPFO is enabled
  mm: add a user_virt_to_phys symbol

 Documentation/admin-guide/kernel-parameters.txt |   2 +
 arch/arm64/Kconfig                              |   1 +
 arch/arm64/include/asm/cacheflush.h             |  11 +
 arch/arm64/mm/Makefile                          |   2 +
 arch/arm64/mm/dma-mapping.c                     |  32 +--
 arch/arm64/mm/flush.c                           |   7 +
 arch/arm64/mm/mmu.c                             |   2 +-
 arch/arm64/mm/xpfo.c                            | 127 +++++++++++
 arch/x86/Kconfig                                |   1 +
 arch/x86/include/asm/pgtable.h                  |  25 +++
 arch/x86/mm/Makefile                            |   1 +
 arch/x86/mm/fault.c                             |   6 +
 arch/x86/mm/pageattr.c                          |  22 +-
 arch/x86/mm/xpfo.c                              | 171 +++++++++++++++
 drivers/misc/Makefile                           |   1 +
 drivers/misc/lkdtm.h                            |   5 +
 drivers/misc/lkdtm_core.c                       |   3 +
 drivers/misc/lkdtm_xpfo.c                       | 194 +++++++++++++++++
 include/linux/highmem.h                         |  15 +-
 include/linux/mm.h                              |   2 +
 include/linux/xpfo.h                            |  79 +++++++
 lib/swiotlb.c                                   |   3 +-
 mm/Makefile                                     |   1 +
 mm/mmap.c                                       |  19 +-
 mm/page_alloc.c                                 |   2 +
 mm/page_ext.c                                   |   4 +
 mm/util.c                                       |  32 +++
 mm/xpfo.c                                       | 273 ++++++++++++++++++++++++
 security/Kconfig                                |  19 ++
 29 files changed, 1005 insertions(+), 57 deletions(-)
 create mode 100644 arch/arm64/mm/xpfo.c
 create mode 100644 arch/x86/mm/xpfo.c
 create mode 100644 drivers/misc/lkdtm_xpfo.c
 create mode 100644 include/linux/xpfo.h
 create mode 100644 mm/xpfo.c

-- 
2.11.0

[toc] | [next] | [standalone]

#1728336 — [PATCH v6 05/11] arm64/mm: Add support for XPFO

From: Juerg Haefliger <juerg.haefliger@canonical.com>

Enable support for eXclusive Page Frame Ownership (XPFO) for arm64 and
provide a hook for updating a single kernel page table entry (which is
required by the generic XPFO code).

v6: use flush_tlb_kernel_range() instead of __flush_tlb_one()

CC: linux-arm-kernel@lists.infradead.org
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
Signed-off-by: Tycho Andersen <tycho@docker.com>
---
 arch/arm64/Kconfig     |  1 +
 arch/arm64/mm/Makefile |  2 ++
 arch/arm64/mm/xpfo.c   | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index dfd908630631..44fa44ef02ec 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -121,6 +121,7 @@ config ARM64
 	select SPARSE_IRQ
 	select SYSCTL_EXCEPTION_TRACE
 	select THREAD_INFO_IN_TASK
+	select ARCH_SUPPORTS_XPFO
 	help
 	  ARM 64-bit (AArch64) Linux support.
 
diff --git a/arch/arm64/mm/Makefile b/arch/arm64/mm/Makefile
index 9b0ba191e48e..22e5cab543d8 100644
--- a/arch/arm64/mm/Makefile
+++ b/arch/arm64/mm/Makefile
@@ -11,3 +11,5 @@ KASAN_SANITIZE_physaddr.o	+= n
 
 obj-$(CONFIG_KASAN)		+= kasan_init.o
 KASAN_SANITIZE_kasan_init.o	:= n
+
+obj-$(CONFIG_XPFO)		+= xpfo.o
diff --git a/arch/arm64/mm/xpfo.c b/arch/arm64/mm/xpfo.c
new file mode 100644
index 000000000000..678e2be848eb
--- /dev/null
+++ b/arch/arm64/mm/xpfo.c
@@ -0,0 +1,58 @@
+/*
+ * Copyright (C) 2017 Hewlett Packard Enterprise Development, L.P.
+ * Copyright (C) 2016 Brown University. All rights reserved.
+ *
+ * Authors:
+ *   Juerg Haefliger <juerg.haefliger@hpe.com>
+ *   Vasileios P. Kemerlis <vpk@cs.brown.edu>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/mm.h>
+#include <linux/module.h>
+
+#include <asm/tlbflush.h>
+
+/*
+ * Lookup the page table entry for a virtual address and return a pointer to
+ * the entry. Based on x86 tree.
+ */
+static pte_t *lookup_address(unsigned long addr)
+{
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+
+	pgd = pgd_offset_k(addr);
+	if (pgd_none(*pgd))
+		return NULL;
+
+	pud = pud_offset(pgd, addr);
+	if (pud_none(*pud))
+		return NULL;
+
+	pmd = pmd_offset(pud, addr);
+	if (pmd_none(*pmd))
+		return NULL;
+
+	return pte_offset_kernel(pmd, addr);
+}
+
+/* Update a single kernel page table entry */
+inline void set_kpte(void *kaddr, struct page *page, pgprot_t prot)
+{
+	pte_t *pte = lookup_address((unsigned long)kaddr);
+
+	set_pte(pte, pfn_pte(page_to_pfn(page), prot));
+}
+
+inline void xpfo_flush_kernel_tlb(struct page *page, int order)
+{
+	unsigned long kaddr = (unsigned long)page_address(page);
+	unsigned long size = PAGE_SIZE;
+
+	flush_tlb_kernel_range(kaddr, kaddr + (1 << order) * size);
+}
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1728666 — Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO

> +/*
> + * Lookup the page table entry for a virtual address and return a pointer to
> + * the entry. Based on x86 tree.
> + */
> +static pte_t *lookup_address(unsigned long addr)

Seems like this should be moved to common arm64 mm code and used by
kernel_page_present.

[toc] | [prev] | [next] | [standalone]

#1729108 — Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO

On Fri, Sep 08, 2017 at 12:53:47AM -0700, Christoph Hellwig wrote:
> > +/*
> > + * Lookup the page table entry for a virtual address and return a pointer to
> > + * the entry. Based on x86 tree.
> > + */
> > +static pte_t *lookup_address(unsigned long addr)
> 
> Seems like this should be moved to common arm64 mm code and used by
> kernel_page_present.

Sounds good, I'll include something like the patch below in the next
series.

Unfortunately, adding an implementation of lookup_address seems to be
slightly more complicated than necessary, because of the xen piece. We
have to define lookup_address() with the level parameter, but it's not
obvious to me to name the page levels. So for now I've just left it as
a WARN() if someone supplies it.

It seems like xen still does need this to be defined, because if I
define it without level:

drivers/xen/xenbus/xenbus_client.c: In function ‘xenbus_unmap_ring_vfree_pv’:
drivers/xen/xenbus/xenbus_client.c:760:4: error: too many arguments to function ‘lookup_address’
    lookup_address(addr, &level)).maddr;
    ^~~~~~~~~~~~~~
In file included from ./arch/arm64/include/asm/page.h:37:0,
                 from ./include/linux/mmzone.h:20,
                 from ./include/linux/gfp.h:5,
                 from ./include/linux/mm.h:9,
                 from drivers/xen/xenbus/xenbus_client.c:33:
./arch/arm64/include/asm/pgtable-types.h:67:15: note: declared here
 extern pte_t *lookup_address(unsigned long addr);
               ^~~~~~~~~~~~~~

I've cc-d the xen folks, maybe they can suggest a way to untangle it?
Alternatively, if someone can suggest a good naming scheme for the
page levels, I can just do that.

Cheers,

Tycho


From 0b3be95873e3e8caa39fa50efc0d06d57fc6eb5e Mon Sep 17 00:00:00 2001
From: Tycho Andersen <tycho@docker.com>
Date: Fri, 8 Sep 2017 10:43:26 -0600
Subject: [PATCH] arm64: add lookup_address()

Similarly to x86, let's add lookup_address() and use it in
kernel_page_present(). We'll use it in the next patch for the
implementation of XPFO as well.

Signed-off-by: Tycho Andersen <tycho@docker.com>
---
 arch/arm64/include/asm/pgtable-types.h |  2 ++
 arch/arm64/mm/pageattr.c               | 47 ++++++++++++++++++++--------------
 include/xen/arm/page.h                 | 10 --------
 3 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable-types.h b/arch/arm64/include/asm/pgtable-types.h
index 345a072b5856..fad3db5a673f 100644
--- a/arch/arm64/include/asm/pgtable-types.h
+++ b/arch/arm64/include/asm/pgtable-types.h
@@ -64,4 +64,6 @@ typedef struct { pteval_t pgprot; } pgprot_t;
 #include <asm-generic/5level-fixup.h>
 #endif
 
+extern pte_t *lookup_address(unsigned long addr, unsigned int *level);
+
 #endif	/* __ASM_PGTABLE_TYPES_H */
diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index a682a0a2a0fa..437a12485873 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c
@@ -138,6 +138,32 @@ int set_memory_valid(unsigned long addr, int numpages, int enable)
 					__pgprot(PTE_VALID));
 }
 
+pte_t *lookup_address(unsigned long addr, unsigned int *level)
+{
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+
+	if (unlikely(level)) {
+		WARN(1, "level unused on arm64\n");
+		*level = 0;
+	}
+
+	pgd = pgd_offset_k(addr);
+	if (pgd_none(*pgd))
+		return NULL;
+
+	pud = pud_offset(pgd, addr);
+	if (pud_none(*pud))
+		return NULL;
+
+	pmd = pmd_offset(pud, addr);
+	if (pmd_none(*pmd))
+		return NULL;
+
+	return pte_offset_kernel(pmd, addr);
+}
+
 #ifdef CONFIG_DEBUG_PAGEALLOC
 void __kernel_map_pages(struct page *page, int numpages, int enable)
 {
@@ -156,29 +182,12 @@ void __kernel_map_pages(struct page *page, int numpages, int enable)
  */
 bool kernel_page_present(struct page *page)
 {
-	pgd_t *pgd;
-	pud_t *pud;
-	pmd_t *pmd;
-	pte_t *pte;
 	unsigned long addr = (unsigned long)page_address(page);
+	pte_t *pte = lookup_address(addr);
 
-	pgd = pgd_offset_k(addr);
-	if (pgd_none(*pgd))
-		return false;
-
-	pud = pud_offset(pgd, addr);
-	if (pud_none(*pud))
-		return false;
-	if (pud_sect(*pud))
-		return true;
-
-	pmd = pmd_offset(pud, addr);
-	if (pmd_none(*pmd))
+	if (!pte)
 		return false;
-	if (pmd_sect(*pmd))
-		return true;
 
-	pte = pte_offset_kernel(pmd, addr);
 	return pte_valid(*pte);
 }
 #endif /* CONFIG_HIBERNATION */
diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
index 415dbc6e43fd..6adc2a955340 100644
--- a/include/xen/arm/page.h
+++ b/include/xen/arm/page.h
@@ -84,16 +84,6 @@ static inline xmaddr_t arbitrary_virt_to_machine(void *vaddr)
 	BUG();
 }
 
-/* TODO: this shouldn't be here but it is because the frontend drivers
- * are using it (its rolled in headers) even though we won't hit the code path.
- * So for right now just punt with this.
- */
-static inline pte_t *lookup_address(unsigned long address, unsigned int *level)
-{
-	BUG();
-	return NULL;
-}
-
 extern int set_foreign_p2m_mapping(struct gnttab_map_grant_ref *map_ops,
 				   struct gnttab_map_grant_ref *kmap_ops,
 				   struct page **pages, unsigned int count);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1732181 — Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO

Hi,

CC Juergen, Boris and Stefano.

On 08/09/17 18:24, Tycho Andersen wrote:
> On Fri, Sep 08, 2017 at 12:53:47AM -0700, Christoph Hellwig wrote:
>>> +/*
>>> + * Lookup the page table entry for a virtual address and return a pointer to
>>> + * the entry. Based on x86 tree.
>>> + */
>>> +static pte_t *lookup_address(unsigned long addr)
>>
>> Seems like this should be moved to common arm64 mm code and used by
>> kernel_page_present.
> 
> Sounds good, I'll include something like the patch below in the next
> series.
> 
> Unfortunately, adding an implementation of lookup_address seems to be
> slightly more complicated than necessary, because of the xen piece. We
> have to define lookup_address() with the level parameter, but it's not
> obvious to me to name the page levels. So for now I've just left it as
> a WARN() if someone supplies it.
> 
> It seems like xen still does need this to be defined, because if I
> define it without level:
> 
> drivers/xen/xenbus/xenbus_client.c: In function ‘xenbus_unmap_ring_vfree_pv’:
> drivers/xen/xenbus/xenbus_client.c:760:4: error: too many arguments to function ‘lookup_address’
>      lookup_address(addr, &level)).maddr;
>      ^~~~~~~~~~~~~~
> In file included from ./arch/arm64/include/asm/page.h:37:0,
>                   from ./include/linux/mmzone.h:20,
>                   from ./include/linux/gfp.h:5,
>                   from ./include/linux/mm.h:9,
>                   from drivers/xen/xenbus/xenbus_client.c:33:
> ./arch/arm64/include/asm/pgtable-types.h:67:15: note: declared here
>   extern pte_t *lookup_address(unsigned long addr);
>                 ^~~~~~~~~~~~~~
> 
> I've cc-d the xen folks, maybe they can suggest a way to untangle it?
> Alternatively, if someone can suggest a good naming scheme for the
> page levels, I can just do that.

The implementation of lookup_address(...) on ARM for Xen (see 
include/xen/arm/page.h) is just a BUG(). This is because this code 
should never be called (only used for x86 PV code).

Furthermore, xenbus client does not use at all the level. It is just to 
cope with the x86 version of lookup_address.

So one way to solve the problem would be to introduce 
xen_lookup_address(addr) that would be implemented as:
	- on x86
		unsigned int level;

		return lookup_address(addr, &level).maddr;
	- on ARM
		BUG();

With that there would be no prototype clash and avoid introducing a 
level parameter.

Cheers,

-- 
Julien Grall

[toc] | [prev] | [next] | [standalone]

#1732228 — Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO

On 14/09/17 12:41, Julien Grall wrote:
> Hi,
> 
> CC Juergen, Boris and Stefano.
> 
> On 08/09/17 18:24, Tycho Andersen wrote:
>> On Fri, Sep 08, 2017 at 12:53:47AM -0700, Christoph Hellwig wrote:
>>>> +/*
>>>> + * Lookup the page table entry for a virtual address and return a
>>>> pointer to
>>>> + * the entry. Based on x86 tree.
>>>> + */
>>>> +static pte_t *lookup_address(unsigned long addr)
>>>
>>> Seems like this should be moved to common arm64 mm code and used by
>>> kernel_page_present.
>>
>> Sounds good, I'll include something like the patch below in the next
>> series.
>>
>> Unfortunately, adding an implementation of lookup_address seems to be
>> slightly more complicated than necessary, because of the xen piece. We
>> have to define lookup_address() with the level parameter, but it's not
>> obvious to me to name the page levels. So for now I've just left it as
>> a WARN() if someone supplies it.
>>
>> It seems like xen still does need this to be defined, because if I
>> define it without level:
>>
>> drivers/xen/xenbus/xenbus_client.c: In function
>> ‘xenbus_unmap_ring_vfree_pv’:
>> drivers/xen/xenbus/xenbus_client.c:760:4: error: too many arguments to
>> function ‘lookup_address’
>>      lookup_address(addr, &level)).maddr;
>>      ^~~~~~~~~~~~~~
>> In file included from ./arch/arm64/include/asm/page.h:37:0,
>>                   from ./include/linux/mmzone.h:20,
>>                   from ./include/linux/gfp.h:5,
>>                   from ./include/linux/mm.h:9,
>>                   from drivers/xen/xenbus/xenbus_client.c:33:
>> ./arch/arm64/include/asm/pgtable-types.h:67:15: note: declared here
>>   extern pte_t *lookup_address(unsigned long addr);
>>                 ^~~~~~~~~~~~~~
>>
>> I've cc-d the xen folks, maybe they can suggest a way to untangle it?
>> Alternatively, if someone can suggest a good naming scheme for the
>> page levels, I can just do that.
> 
> The implementation of lookup_address(...) on ARM for Xen (see
> include/xen/arm/page.h) is just a BUG(). This is because this code
> should never be called (only used for x86 PV code).
> 
> Furthermore, xenbus client does not use at all the level. It is just to
> cope with the x86 version of lookup_address.
> 
> So one way to solve the problem would be to introduce
> xen_lookup_address(addr) that would be implemented as:
>     - on x86
>         unsigned int level;
> 
>         return lookup_address(addr, &level).maddr;
>     - on ARM
>         BUG();
> 
> With that there would be no prototype clash and avoid introducing a
> level parameter.

I'd rather add some #ifdef CONFIG_XEN_PV and remove the *_pv functions
from ARM completely this way. I'm sending a patch soon...


Juergen

[toc] | [prev] | [next] | [standalone]

#1732493 — Re: [kernel-hardening] [PATCH v6 05/11] arm64/mm: Add support for XPFO

Hi,

On Thu, Sep 07, 2017 at 11:36:03AM -0600, Tycho Andersen wrote:
> From: Juerg Haefliger <juerg.haefliger@canonical.com>
> 
> Enable support for eXclusive Page Frame Ownership (XPFO) for arm64 and
> provide a hook for updating a single kernel page table entry (which is
> required by the generic XPFO code).
> 
> v6: use flush_tlb_kernel_range() instead of __flush_tlb_one()
> 
> CC: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
> Signed-off-by: Tycho Andersen <tycho@docker.com>
> ---
>  arch/arm64/Kconfig     |  1 +
>  arch/arm64/mm/Makefile |  2 ++
>  arch/arm64/mm/xpfo.c   | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index dfd908630631..44fa44ef02ec 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -121,6 +121,7 @@ config ARM64
>  	select SPARSE_IRQ
>  	select SYSCTL_EXCEPTION_TRACE
>  	select THREAD_INFO_IN_TASK
> +	select ARCH_SUPPORTS_XPFO

A bit of a nit, but this list is (intended to be) organised alphabetically.
Could you please try to retain that?

i.e. place this between ARCH_SUPPORTS_NUMA_BALANCING and
ARCH_WANT_COMPAT_IPC_PARSE_VERSION.

>  	help
>  	  ARM 64-bit (AArch64) Linux support.
>  
> diff --git a/arch/arm64/mm/Makefile b/arch/arm64/mm/Makefile
> index 9b0ba191e48e..22e5cab543d8 100644
> --- a/arch/arm64/mm/Makefile
> +++ b/arch/arm64/mm/Makefile
> @@ -11,3 +11,5 @@ KASAN_SANITIZE_physaddr.o	+= n
>  
>  obj-$(CONFIG_KASAN)		+= kasan_init.o
>  KASAN_SANITIZE_kasan_init.o	:= n
> +
> +obj-$(CONFIG_XPFO)		+= xpfo.o
> diff --git a/arch/arm64/mm/xpfo.c b/arch/arm64/mm/xpfo.c
> new file mode 100644
> index 000000000000..678e2be848eb
> --- /dev/null
> +++ b/arch/arm64/mm/xpfo.c
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright (C) 2017 Hewlett Packard Enterprise Development, L.P.
> + * Copyright (C) 2016 Brown University. All rights reserved.
> + *
> + * Authors:
> + *   Juerg Haefliger <juerg.haefliger@hpe.com>
> + *   Vasileios P. Kemerlis <vpk@cs.brown.edu>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +
> +#include <asm/tlbflush.h>
> +
> +/*
> + * Lookup the page table entry for a virtual address and return a pointer to
> + * the entry. Based on x86 tree.
> + */

Is this intended for kernel VAs, user VAs, or both?

There are different constraints for fiddling with either (e.g. holding
mmap_sem), so we should be clear regarding the intended use-case.

> +static pte_t *lookup_address(unsigned long addr)
> +{
> +	pgd_t *pgd;
> +	pud_t *pud;
> +	pmd_t *pmd;
> +
> +	pgd = pgd_offset_k(addr);
> +	if (pgd_none(*pgd))
> +		return NULL;
> +
> +	pud = pud_offset(pgd, addr);
> +	if (pud_none(*pud))
> +		return NULL;

What if it's not none, but not a table?

I think we chould check pud_sect() here, and/or pud_bad().

> +
> +	pmd = pmd_offset(pud, addr);
> +	if (pmd_none(*pmd))
> +		return NULL;

Likewise.

> +
> +	return pte_offset_kernel(pmd, addr);
> +}

Given this expects a pte, it might make more sense to call this
lookup_address_pte() to make that clear.

> +
> +/* Update a single kernel page table entry */
> +inline void set_kpte(void *kaddr, struct page *page, pgprot_t prot)
> +{
> +	pte_t *pte = lookup_address((unsigned long)kaddr);
> +
> +	set_pte(pte, pfn_pte(page_to_pfn(page), prot));

We can get NULL from lookup_address(), so this doesn't look right.

If NULL implies an error, drop a BUG_ON(!pte) before the set_pte.

> +}
> +
> +inline void xpfo_flush_kernel_tlb(struct page *page, int order)
> +{
> +	unsigned long kaddr = (unsigned long)page_address(page);
> +	unsigned long size = PAGE_SIZE;

unsigned long size = PAGE_SIZE << order;

> +
> +	flush_tlb_kernel_range(kaddr, kaddr + (1 << order) * size);

... and this can be simpler.

I haven't brought myself back up to speed, so it might not be possible, but I
still think it would be preferable for XPFO to call flush_tlb_kernel_range()
directly.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1734490 — Re: [kernel-hardening] [PATCH v6 05/11] arm64/mm: Add support for XPFO

Hi Mark,

On Thu, Sep 14, 2017 at 07:22:08PM +0100, Mark Rutland wrote:
> Hi,
> 
> On Thu, Sep 07, 2017 at 11:36:03AM -0600, Tycho Andersen wrote:
> > From: Juerg Haefliger <juerg.haefliger@canonical.com>
> > 
> > Enable support for eXclusive Page Frame Ownership (XPFO) for arm64 and
> > provide a hook for updating a single kernel page table entry (which is
> > required by the generic XPFO code).
> > 
> > v6: use flush_tlb_kernel_range() instead of __flush_tlb_one()
> > 
> > CC: linux-arm-kernel@lists.infradead.org
> > Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
> > Signed-off-by: Tycho Andersen <tycho@docker.com>
> > ---
> >  arch/arm64/Kconfig     |  1 +
> >  arch/arm64/mm/Makefile |  2 ++
> >  arch/arm64/mm/xpfo.c   | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 61 insertions(+)
> > 
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index dfd908630631..44fa44ef02ec 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -121,6 +121,7 @@ config ARM64
> >  	select SPARSE_IRQ
> >  	select SYSCTL_EXCEPTION_TRACE
> >  	select THREAD_INFO_IN_TASK
> > +	select ARCH_SUPPORTS_XPFO
> 
> A bit of a nit, but this list is (intended to be) organised alphabetically.
> Could you please try to retain that?
> 
> i.e. place this between ARCH_SUPPORTS_NUMA_BALANCING and
> ARCH_WANT_COMPAT_IPC_PARSE_VERSION.

Will do.

> >  	help
> >  	  ARM 64-bit (AArch64) Linux support.
> >  
> > diff --git a/arch/arm64/mm/Makefile b/arch/arm64/mm/Makefile
> > index 9b0ba191e48e..22e5cab543d8 100644
> > --- a/arch/arm64/mm/Makefile
> > +++ b/arch/arm64/mm/Makefile
> > @@ -11,3 +11,5 @@ KASAN_SANITIZE_physaddr.o	+= n
> >  
> >  obj-$(CONFIG_KASAN)		+= kasan_init.o
> >  KASAN_SANITIZE_kasan_init.o	:= n
> > +
> > +obj-$(CONFIG_XPFO)		+= xpfo.o
> > diff --git a/arch/arm64/mm/xpfo.c b/arch/arm64/mm/xpfo.c
> > new file mode 100644
> > index 000000000000..678e2be848eb
> > --- /dev/null
> > +++ b/arch/arm64/mm/xpfo.c
> > @@ -0,0 +1,58 @@
> > +/*
> > + * Copyright (C) 2017 Hewlett Packard Enterprise Development, L.P.
> > + * Copyright (C) 2016 Brown University. All rights reserved.
> > + *
> > + * Authors:
> > + *   Juerg Haefliger <juerg.haefliger@hpe.com>
> > + *   Vasileios P. Kemerlis <vpk@cs.brown.edu>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as published by
> > + * the Free Software Foundation.
> > + */
> > +
> > +#include <linux/mm.h>
> > +#include <linux/module.h>
> > +
> > +#include <asm/tlbflush.h>
> > +
> > +/*
> > + * Lookup the page table entry for a virtual address and return a pointer to
> > + * the entry. Based on x86 tree.
> > + */
> 
> Is this intended for kernel VAs, user VAs, or both?
> 
> There are different constraints for fiddling with either (e.g. holding
> mmap_sem), so we should be clear regarding the intended use-case.

kernel only; I can add a comment noting this.

> > +static pte_t *lookup_address(unsigned long addr)
> > +{
> > +	pgd_t *pgd;
> > +	pud_t *pud;
> > +	pmd_t *pmd;
> > +
> > +	pgd = pgd_offset_k(addr);
> > +	if (pgd_none(*pgd))
> > +		return NULL;
> > +
> > +	pud = pud_offset(pgd, addr);
> > +	if (pud_none(*pud))
> > +		return NULL;
> 
> What if it's not none, but not a table?
> 
> I think we chould check pud_sect() here, and/or pud_bad().
> 
> > +
> > +	pmd = pmd_offset(pud, addr);
> > +	if (pmd_none(*pmd))
> > +		return NULL;
> 
> Likewise.

In principle pud_sect() should be okay, because we say that XPFO
doesn't support section mappings yet. I'll add a check for pud_bad().

However, Christoph suggested that we move this to common code and
there it won't be okay.

> > +
> > +	return pte_offset_kernel(pmd, addr);
> > +}
> 
> Given this expects a pte, it might make more sense to call this
> lookup_address_pte() to make that clear.
> 
> > +
> > +/* Update a single kernel page table entry */
> > +inline void set_kpte(void *kaddr, struct page *page, pgprot_t prot)
> > +{
> > +	pte_t *pte = lookup_address((unsigned long)kaddr);
> > +
> > +	set_pte(pte, pfn_pte(page_to_pfn(page), prot));
> 
> We can get NULL from lookup_address(), so this doesn't look right.
> 
> If NULL implies an error, drop a BUG_ON(!pte) before the set_pte.

It does, I'll add this (as a WARN() and then no-op), thanks.

> > +}
> > +
> > +inline void xpfo_flush_kernel_tlb(struct page *page, int order)
> > +{
> > +	unsigned long kaddr = (unsigned long)page_address(page);
> > +	unsigned long size = PAGE_SIZE;
> 
> unsigned long size = PAGE_SIZE << order;
> 
> > +
> > +	flush_tlb_kernel_range(kaddr, kaddr + (1 << order) * size);
> 
> ... and this can be simpler.
> 
> I haven't brought myself back up to speed, so it might not be possible, but I
> still think it would be preferable for XPFO to call flush_tlb_kernel_range()
> directly.

I don't think we can, since on x86 it uses smp functions, and in some
cases those aren't safe.

Cheers,

Tycho

[toc] | [prev] | [next] | [standalone]

#1728337 — [PATCH v6 01/11] mm: add MAP_HUGETLB support to vm_mmap

vm_mmap is exported, which means kernel modules can use it. In particular,
for testing XPFO support, we want to use it with the MAP_HUGETLB flag, so
let's support it via vm_mmap.

Signed-off-by: Tycho Andersen <tycho@docker.com>
Tested-by: Marco Benatto <marco.antonio.780@gmail.com>
---
 include/linux/mm.h |  2 ++
 mm/mmap.c          | 19 +------------------
 mm/util.c          | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index c1f6c95f3496..5dfe009adcb9 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2141,6 +2141,8 @@ struct vm_unmapped_area_info {
 extern unsigned long unmapped_area(struct vm_unmapped_area_info *info);
 extern unsigned long unmapped_area_topdown(struct vm_unmapped_area_info *info);
 
+struct file *map_hugetlb_setup(unsigned long *len, unsigned long flags);
+
 /*
  * Search for an unmapped address range.
  *
diff --git a/mm/mmap.c b/mm/mmap.c
index f19efcf75418..f24fc14808e1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1490,24 +1490,7 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 		if (unlikely(flags & MAP_HUGETLB && !is_file_hugepages(file)))
 			goto out_fput;
 	} else if (flags & MAP_HUGETLB) {
-		struct user_struct *user = NULL;
-		struct hstate *hs;
-
-		hs = hstate_sizelog((flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
-		if (!hs)
-			return -EINVAL;
-
-		len = ALIGN(len, huge_page_size(hs));
-		/*
-		 * VM_NORESERVE is used because the reservations will be
-		 * taken when vm_ops->mmap() is called
-		 * A dummy user value is used because we are not locking
-		 * memory so no accounting is necessary
-		 */
-		file = hugetlb_file_setup(HUGETLB_ANON_FILE, len,
-				VM_NORESERVE,
-				&user, HUGETLB_ANONHUGE_INODE,
-				(flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
+		file = map_hugetlb_setup(&len, flags);
 		if (IS_ERR(file))
 			return PTR_ERR(file);
 	}
diff --git a/mm/util.c b/mm/util.c
index 9ecddf568fe3..93c253512aaa 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -340,6 +340,29 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
 	return ret;
 }
 
+struct file *map_hugetlb_setup(unsigned long *len, unsigned long flags)
+{
+	struct user_struct *user = NULL;
+	struct hstate *hs;
+
+	hs = hstate_sizelog((flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
+	if (!hs)
+		return ERR_PTR(-EINVAL);
+
+	*len = ALIGN(*len, huge_page_size(hs));
+
+	/*
+	 * VM_NORESERVE is used because the reservations will be
+	 * taken when vm_ops->mmap() is called
+	 * A dummy user value is used because we are not locking
+	 * memory so no accounting is necessary
+	 */
+	return hugetlb_file_setup(HUGETLB_ANON_FILE, *len,
+			VM_NORESERVE,
+			&user, HUGETLB_ANONHUGE_INODE,
+			(flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
+}
+
 unsigned long vm_mmap(struct file *file, unsigned long addr,
 	unsigned long len, unsigned long prot,
 	unsigned long flag, unsigned long offset)
@@ -349,6 +372,15 @@ unsigned long vm_mmap(struct file *file, unsigned long addr,
 	if (unlikely(offset_in_page(offset)))
 		return -EINVAL;
 
+	if (flag & MAP_HUGETLB) {
+		if (file)
+			return -EINVAL;
+
+		file = map_hugetlb_setup(&len, flag);
+		if (IS_ERR(file))
+			return PTR_ERR(file);
+	}
+
 	return vm_mmap_pgoff(file, addr, len, prot, flag, offset >> PAGE_SHIFT);
 }
 EXPORT_SYMBOL(vm_mmap);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1728665 — Re: [PATCH v6 01/11] mm: add MAP_HUGETLB support to vm_mmap

On Thu, Sep 07, 2017 at 11:35:59AM -0600, Tycho Andersen wrote:
> vm_mmap is exported, which means kernel modules can use it. In particular,
> for testing XPFO support, we want to use it with the MAP_HUGETLB flag, so
> let's support it via vm_mmap.

>  	} else if (flags & MAP_HUGETLB) {
> +		file = map_hugetlb_setup(&len, flags);
>  		if (IS_ERR(file))
>  			return PTR_ERR(file);
>  	}

It seems like you should remove this hunk entirely and make all
MAP_HUGETLB calls go through vm_mmap.

[toc] | [prev] | [next] | [standalone]

#1728338 — [PATCH v6 02/11] x86: always set IF before oopsing from page fault

Oopsing might kill the task, via rewind_stack_do_exit() at the bottom, and
that might sleep:

Aug 23 19:30:27 xpfo kernel: [   38.302714] BUG: sleeping function called from invalid context at ./include/linux/percpu-rwsem.h:33
Aug 23 19:30:27 xpfo kernel: [   38.303837] in_atomic(): 0, irqs_disabled(): 1, pid: 1970, name: lkdtm_xpfo_test
Aug 23 19:30:27 xpfo kernel: [   38.304758] CPU: 3 PID: 1970 Comm: lkdtm_xpfo_test Tainted: G      D         4.13.0-rc5+ #228
Aug 23 19:30:27 xpfo kernel: [   38.305813] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.1-1ubuntu1 04/01/2014
Aug 23 19:30:27 xpfo kernel: [   38.306926] Call Trace:
Aug 23 19:30:27 xpfo kernel: [   38.307243]  dump_stack+0x63/0x8b
Aug 23 19:30:27 xpfo kernel: [   38.307665]  ___might_sleep+0xec/0x110
Aug 23 19:30:27 xpfo kernel: [   38.308139]  __might_sleep+0x45/0x80
Aug 23 19:30:27 xpfo kernel: [   38.308593]  exit_signals+0x21/0x1c0
Aug 23 19:30:27 xpfo kernel: [   38.309046]  ? blocking_notifier_call_chain+0x11/0x20
Aug 23 19:30:27 xpfo kernel: [   38.309677]  do_exit+0x98/0xbf0
Aug 23 19:30:27 xpfo kernel: [   38.310078]  ? smp_reader+0x27/0x40 [lkdtm]
Aug 23 19:30:27 xpfo kernel: [   38.310604]  ? kthread+0x10f/0x150
Aug 23 19:30:27 xpfo kernel: [   38.311045]  ? read_user_with_flags+0x60/0x60 [lkdtm]
Aug 23 19:30:27 xpfo kernel: [   38.311680]  rewind_stack_do_exit+0x17/0x20

To be safe, let's just always enable irqs.

The particular case I'm hitting is:

Aug 23 19:30:27 xpfo kernel: [   38.278615]  __bad_area_nosemaphore+0x1a9/0x1d0
Aug 23 19:30:27 xpfo kernel: [   38.278617]  bad_area_nosemaphore+0xf/0x20
Aug 23 19:30:27 xpfo kernel: [   38.278618]  __do_page_fault+0xd1/0x540
Aug 23 19:30:27 xpfo kernel: [   38.278620]  ? irq_work_queue+0x9b/0xb0
Aug 23 19:30:27 xpfo kernel: [   38.278623]  ? wake_up_klogd+0x36/0x40
Aug 23 19:30:27 xpfo kernel: [   38.278624]  trace_do_page_fault+0x3c/0xf0
Aug 23 19:30:27 xpfo kernel: [   38.278625]  do_async_page_fault+0x14/0x60
Aug 23 19:30:27 xpfo kernel: [   38.278627]  async_page_fault+0x28/0x30

When a fault is in kernel space which has been triggered by XPFO.

Signed-off-by: Tycho Andersen <tycho@docker.com>
CC: x86@kernel.org
---
 arch/x86/mm/fault.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 2a1fa10c6a98..7572ad4dae70 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -864,6 +864,12 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 	/* Executive summary in case the body of the oops scrolled away */
 	printk(KERN_DEFAULT "CR2: %016lx\n", address);
 
+	/*
+	 * We're about to oops, which might kill the task. Make sure we're
+	 * allowed to sleep.
+	 */
+	flags |= X86_EFLAGS_IF;
+
 	oops_end(flags, regs, sig);
 }
 
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1730269

Hi Tycho ,

On 2017/9/8 1:35, Tycho Andersen wrote:
> Hi all,
> 
> Here is v6 of the XPFO set; see v5 discussion here:
> https://lkml.org/lkml/2017/8/9/803
> 
> Changelogs are in the individual patch notes, but the highlights are:
> * add primitives for ensuring memory areas are mapped (although these are quite
>   ugly, using stack allocation; I'm open to better suggestions)
> * instead of not flushing caches, re-map pages using the above
> * TLB flushing is much more correct (i.e. we're always flushing everything
>   everywhere). I suspect we may be able to back this off in some cases, but I'm
>   still trying to collect performance numbers to prove this is worth doing.
> 
> I have no TODOs left for this set myself, other than fixing whatever review
> feedback people have. Thoughts and testing welcome!

According to the paper of Vasileios P. Kemerlis et al, the mainline kernel
will not set the Pro. of physmap(direct map area) to RW(X), so do we really
need XPFO to protect from ret2dir attack?

Thanks
Yisheng xie

> 
> Cheers,
> 
> Tycho
> 
> Juerg Haefliger (6):
>   mm, x86: Add support for eXclusive Page Frame Ownership (XPFO)
>   swiotlb: Map the buffer if it was unmapped by XPFO
>   arm64/mm: Add support for XPFO
>   arm64/mm, xpfo: temporarily map dcache regions
>   arm64/mm: Add support for XPFO to swiotlb
>   lkdtm: Add test for XPFO
> 
> Tycho Andersen (5):
>   mm: add MAP_HUGETLB support to vm_mmap
>   x86: always set IF before oopsing from page fault
>   xpfo: add primitives for mapping underlying memory
>   arm64/mm: disable section/contiguous mappings if XPFO is enabled
>   mm: add a user_virt_to_phys symbol
> 
>  Documentation/admin-guide/kernel-parameters.txt |   2 +
>  arch/arm64/Kconfig                              |   1 +
>  arch/arm64/include/asm/cacheflush.h             |  11 +
>  arch/arm64/mm/Makefile                          |   2 +
>  arch/arm64/mm/dma-mapping.c                     |  32 +--
>  arch/arm64/mm/flush.c                           |   7 +
>  arch/arm64/mm/mmu.c                             |   2 +-
>  arch/arm64/mm/xpfo.c                            | 127 +++++++++++
>  arch/x86/Kconfig                                |   1 +
>  arch/x86/include/asm/pgtable.h                  |  25 +++
>  arch/x86/mm/Makefile                            |   1 +
>  arch/x86/mm/fault.c                             |   6 +
>  arch/x86/mm/pageattr.c                          |  22 +-
>  arch/x86/mm/xpfo.c                              | 171 +++++++++++++++
>  drivers/misc/Makefile                           |   1 +
>  drivers/misc/lkdtm.h                            |   5 +
>  drivers/misc/lkdtm_core.c                       |   3 +
>  drivers/misc/lkdtm_xpfo.c                       | 194 +++++++++++++++++
>  include/linux/highmem.h                         |  15 +-
>  include/linux/mm.h                              |   2 +
>  include/linux/xpfo.h                            |  79 +++++++
>  lib/swiotlb.c                                   |   3 +-
>  mm/Makefile                                     |   1 +
>  mm/mmap.c                                       |  19 +-
>  mm/page_alloc.c                                 |   2 +
>  mm/page_ext.c                                   |   4 +
>  mm/util.c                                       |  32 +++
>  mm/xpfo.c                                       | 273 ++++++++++++++++++++++++
>  security/Kconfig                                |  19 ++
>  29 files changed, 1005 insertions(+), 57 deletions(-)
>  create mode 100644 arch/arm64/mm/xpfo.c
>  create mode 100644 arch/x86/mm/xpfo.c
>  create mode 100644 drivers/misc/lkdtm_xpfo.c
>  create mode 100644 include/linux/xpfo.h
>  create mode 100644 mm/xpfo.c
> 

[toc] | [prev] | [next] | [standalone]

#1730383

Hi Yisheng,

On Mon, Sep 11, 2017 at 06:34:45PM +0800, Yisheng Xie wrote:
> Hi Tycho ,
> 
> On 2017/9/8 1:35, Tycho Andersen wrote:
> > Hi all,
> > 
> > Here is v6 of the XPFO set; see v5 discussion here:
> > https://lkml.org/lkml/2017/8/9/803
> > 
> > Changelogs are in the individual patch notes, but the highlights are:
> > * add primitives for ensuring memory areas are mapped (although these are quite
> >   ugly, using stack allocation; I'm open to better suggestions)
> > * instead of not flushing caches, re-map pages using the above
> > * TLB flushing is much more correct (i.e. we're always flushing everything
> >   everywhere). I suspect we may be able to back this off in some cases, but I'm
> >   still trying to collect performance numbers to prove this is worth doing.
> > 
> > I have no TODOs left for this set myself, other than fixing whatever review
> > feedback people have. Thoughts and testing welcome!
> 
> According to the paper of Vasileios P. Kemerlis et al, the mainline kernel
> will not set the Pro. of physmap(direct map area) to RW(X), so do we really
> need XPFO to protect from ret2dir attack?

I guess you're talking about section 4.3? They mention that that x86
only gets rw, but that aarch64 is rwx still.

But in either case this still provides access protection, similar to
SMAP. Also, if I understand things correctly the protections are
unmanaged, so a page that had the +x bit set at some point, it could
be used for ret2dir.

Cheers,

Tycho

[toc] | [prev] | [next] | [standalone]

#1730694

Hi Tycho,

On 2017/9/11 23:02, Tycho Andersen wrote:
> Hi Yisheng,
> 
> On Mon, Sep 11, 2017 at 06:34:45PM +0800, Yisheng Xie wrote:
>> Hi Tycho ,
>>
>> On 2017/9/8 1:35, Tycho Andersen wrote:
>>> Hi all,
>>>
>>> Here is v6 of the XPFO set; see v5 discussion here:
>>> https://lkml.org/lkml/2017/8/9/803
>>>
>>> Changelogs are in the individual patch notes, but the highlights are:
>>> * add primitives for ensuring memory areas are mapped (although these are quite
>>>   ugly, using stack allocation; I'm open to better suggestions)
>>> * instead of not flushing caches, re-map pages using the above
>>> * TLB flushing is much more correct (i.e. we're always flushing everything
>>>   everywhere). I suspect we may be able to back this off in some cases, but I'm
>>>   still trying to collect performance numbers to prove this is worth doing.
>>>
>>> I have no TODOs left for this set myself, other than fixing whatever review
>>> feedback people have. Thoughts and testing welcome!
>>
>> According to the paper of Vasileios P. Kemerlis et al, the mainline kernel
>> will not set the Pro. of physmap(direct map area) to RW(X), so do we really
>> need XPFO to protect from ret2dir attack?
> 
> I guess you're talking about section 4.3? 
Yes

> They mention that that x86
> only gets rw, but that aarch64 is rwx still.
IIRC, the in kernel of v4.13 the aarch64 is not rwx, I will check it.

> 
> But in either case this still provides access protection, similar to
> SMAP. Also, if I understand things correctly the protections are
> unmanaged, so a page that had the +x bit set at some point, it could
> be used for ret2dir.
So you means that the Pro. of direct map area maybe changed to +x, then ret2dir attack can use it?

Thanks
Yisheng Xie

[toc] | [prev] | [next] | [standalone]

#1730718

On 09/12/2017 09:07 AM, Yisheng Xie wrote:
> Hi Tycho,
> 
> On 2017/9/11 23:02, Tycho Andersen wrote:
>> Hi Yisheng,
>>
>> On Mon, Sep 11, 2017 at 06:34:45PM +0800, Yisheng Xie wrote:
>>> Hi Tycho ,
>>>
>>> On 2017/9/8 1:35, Tycho Andersen wrote:
>>>> Hi all,
>>>>
>>>> Here is v6 of the XPFO set; see v5 discussion here:
>>>> https://lkml.org/lkml/2017/8/9/803
>>>>
>>>> Changelogs are in the individual patch notes, but the highlights are:
>>>> * add primitives for ensuring memory areas are mapped (although these are quite
>>>>   ugly, using stack allocation; I'm open to better suggestions)
>>>> * instead of not flushing caches, re-map pages using the above
>>>> * TLB flushing is much more correct (i.e. we're always flushing everything
>>>>   everywhere). I suspect we may be able to back this off in some cases, but I'm
>>>>   still trying to collect performance numbers to prove this is worth doing.
>>>>
>>>> I have no TODOs left for this set myself, other than fixing whatever review
>>>> feedback people have. Thoughts and testing welcome!
>>>
>>> According to the paper of Vasileios P. Kemerlis et al, the mainline kernel
>>> will not set the Pro. of physmap(direct map area) to RW(X), so do we really
>>> need XPFO to protect from ret2dir attack?
>>
>> I guess you're talking about section 4.3? 
> Yes
> 
>> They mention that that x86
>> only gets rw, but that aarch64 is rwx still.
> IIRC, the in kernel of v4.13 the aarch64 is not rwx, I will check it.
> 
>>
>> But in either case this still provides access protection, similar to
>> SMAP. Also, if I understand things correctly the protections are
>> unmanaged, so a page that had the +x bit set at some point, it could
>> be used for ret2dir.
> So you means that the Pro. of direct map area maybe changed to +x, then ret2dir attack can use it?

XPFO protects against malicious reads from userspace (potentially
accessing sensitive data). I've also been told by a security expert that
ROP attacks are still possible even if user space memory is
non-executable. XPFO is supposed to prevent that but I haven't been able
to confirm this. It's way out of my comfort zone.

...Juerg


> Thanks
> Yisheng Xie
> 
> 

[toc] | [prev] | [next] | [standalone]

#1730729

On 2017/9/12 15:40, Juerg Haefliger wrote:
> 
> 
> On 09/12/2017 09:07 AM, Yisheng Xie wrote:
>> Hi Tycho,
>>
>> On 2017/9/11 23:02, Tycho Andersen wrote:
>>> Hi Yisheng,
>>>
>>> On Mon, Sep 11, 2017 at 06:34:45PM +0800, Yisheng Xie wrote:
>>>> Hi Tycho ,
>>>>
>>>> On 2017/9/8 1:35, Tycho Andersen wrote:
>>>>> Hi all,
>>>>>
>>>>> Here is v6 of the XPFO set; see v5 discussion here:
>>>>> https://lkml.org/lkml/2017/8/9/803
>>>>>
>>>>> Changelogs are in the individual patch notes, but the highlights are:
>>>>> * add primitives for ensuring memory areas are mapped (although these are quite
>>>>>   ugly, using stack allocation; I'm open to better suggestions)
>>>>> * instead of not flushing caches, re-map pages using the above
>>>>> * TLB flushing is much more correct (i.e. we're always flushing everything
>>>>>   everywhere). I suspect we may be able to back this off in some cases, but I'm
>>>>>   still trying to collect performance numbers to prove this is worth doing.
>>>>>
>>>>> I have no TODOs left for this set myself, other than fixing whatever review
>>>>> feedback people have. Thoughts and testing welcome!
>>>>
>>>> According to the paper of Vasileios P. Kemerlis et al, the mainline kernel
>>>> will not set the Pro. of physmap(direct map area) to RW(X), so do we really
>>>> need XPFO to protect from ret2dir attack?
>>>
>>> I guess you're talking about section 4.3? 
>> Yes
>>
>>> They mention that that x86
>>> only gets rw, but that aarch64 is rwx still.
>> IIRC, the in kernel of v4.13 the aarch64 is not rwx, I will check it.
>>
>>>
>>> But in either case this still provides access protection, similar to
>>> SMAP. Also, if I understand things correctly the protections are
>>> unmanaged, so a page that had the +x bit set at some point, it could
>>> be used for ret2dir.
>> So you means that the Pro. of direct map area maybe changed to +x, then ret2dir attack can use it?
> 
> XPFO protects against malicious reads from userspace (potentially
> accessing sensitive data). 
This sounds reasonable to me.

> I've also been told by a security expert that
> ROP attacks are still possible even if user space memory is
> non-executable. XPFO is supposed to prevent that but I haven't been able
> to confirm this. It's way out of my comfort zone.
It also quite out of knowledge, and I just try hard to understand it. Thanks so much for
your kind explain.  And hope some security expert can give some more detail explain?

Thanks
Yisheng Xie

> 
> ...Juerg

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web