[PATCHv2 0/4] Page protections for arm64

[PATCHv2 0/4] Page protections for arm64

[Laura Abbott]

Hi,

This is v2 of adding page protections for arm64. This implements
CONFIG_DEBUG_SET_MODULE_RONX for modules and adds similar protections
for direct mapped memory. The module portion (1/4) should be ready for
merging assuming no more issues are found (I know I'm off cycle here).

v2:
 - Added a memory barrier after flush_tlb_kernel_range per request of
   Steve Capper.
 - Dropped the ENTRY per Will in handle_arch_irq
 - Added the Kconfig to align sections up to SECTION_SIZE. Added a patch
   to fix the related relocation errors from this option.

Laura Abbott (4):
  arm64: Add CONFIG_DEBUG_SET_MODULE_RONX support
  arm64: Treat handle_arch_irq as a function pointer
  arm64: Switch to ldr for loading the stub vectors
  arm64: add better page protections to arm64

 arch/arm64/Kconfig.debug            |  34 +++++++
 arch/arm64/include/asm/cacheflush.h |   4 +
 arch/arm64/kernel/entry.S           |   8 +-
 arch/arm64/kernel/head.S            |   2 +-
 arch/arm64/kernel/vmlinux.lds.S     |  17 ++++
 arch/arm64/mm/Makefile              |   2 +-
 arch/arm64/mm/init.c                |   1 +
 arch/arm64/mm/mm.h                  |   2 +
 arch/arm64/mm/mmu.c                 | 173 ++++++++++++++++++++++++++++++++----
 arch/arm64/mm/pageattr.c            | 121 +++++++++++++++++++++++++
 10 files changed, 344 insertions(+), 20 deletions(-)
 create mode 100644 arch/arm64/mm/pageattr.c

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

[PATCHv2 1/4] arm64: Add CONFIG_DEBUG_SET_MODULE_RONX support

[Laura Abbott]

In a similar fashion to other architecture, add the infrastructure
and Kconfig to enable DEBUG_SET_MODULE_RONX support. When
enabled, module ranges will be marked read-only/no-execute as
appropriate.

Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 arch/arm64/Kconfig.debug            |  11 ++++
 arch/arm64/include/asm/cacheflush.h |   4 ++
 arch/arm64/mm/Makefile              |   2 +-
 arch/arm64/mm/pageattr.c            | 121 ++++++++++++++++++++++++++++++++++++
 4 files changed, 137 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/mm/pageattr.c

diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index d10ec33..53979ac 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -37,4 +37,15 @@ config PID_IN_CONTEXTIDR
 	  instructions during context switch. Say Y here only if you are
 	  planning to use hardware trace tools with this kernel.
 
+config DEBUG_SET_MODULE_RONX
+        bool "Set loadable kernel module data as NX and text as RO"
+        depends on MODULES
+        help
+          This option helps catch unintended modifications to loadable
+          kernel module's text and read-only data. It also prevents execution
+          of module data. Such protection may interfere with run-time code
+          patching and dynamic kernel tracing - and they might also protect
+          against certain classes of kernel exploits.
+          If in doubt, say "N".
+
 endmenu
diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h
index 4c60e64..c12f837 100644
--- a/arch/arm64/include/asm/cacheflush.h
+++ b/arch/arm64/include/asm/cacheflush.h
@@ -157,4 +157,8 @@ static inline void flush_cache_vunmap(unsigned long start, unsigned long end)
 {
 }
 
+int set_memory_ro(unsigned long addr, int numpages);
+int set_memory_rw(unsigned long addr, int numpages);
+int set_memory_x(unsigned long addr, int numpages);
+int set_memory_nx(unsigned long addr, int numpages);
 #endif
diff --git a/arch/arm64/mm/Makefile b/arch/arm64/mm/Makefile
index b51d364..25b1114 100644
--- a/arch/arm64/mm/Makefile
+++ b/arch/arm64/mm/Makefile
@@ -1,5 +1,5 @@
 obj-y				:= dma-mapping.o extable.o fault.o init.o \
 				   cache.o copypage.o flush.o \
 				   ioremap.o mmap.o pgd.o mmu.o \
-				   context.o tlb.o proc.o
+				   context.o tlb.o proc.o pageattr.o
 obj-$(CONFIG_HUGETLB_PAGE)	+= hugetlbpage.o
diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
new file mode 100644
index 0000000..d8ab747
--- /dev/null
+++ b/arch/arm64/mm/pageattr.c
@@ -0,0 +1,121 @@
+/*
+ * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/sched.h>
+
+#include <asm/pgtable.h>
+#include <asm/tlbflush.h>
+
+static pte_t clear_pte_bit(pte_t pte, pgprot_t prot)
+{
+	pte_val(pte) &= ~pgprot_val(prot);
+	return pte;
+}
+
+static pte_t set_pte_bit(pte_t pte, pgprot_t prot)
+{
+	pte_val(pte) |= pgprot_val(prot);
+	return pte;
+}
+
+static int __change_memory(pte_t *ptep, pgtable_t token, unsigned long addr,
+			pgprot_t prot, bool set)
+{
+	pte_t pte;
+
+	if (set)
+		pte = set_pte_bit(*ptep, prot);
+	else
+		pte = clear_pte_bit(*ptep, prot);
+	set_pte(ptep, pte);
+	return 0;
+}
+
+static int set_page_range(pte_t *ptep, pgtable_t token, unsigned long addr,
+			void *data)
+{
+	pgprot_t prot = (pgprot_t)data;
+
+	return __change_memory(ptep, token, addr, prot, true);
+}
+
+static int clear_page_range(pte_t *ptep, pgtable_t token, unsigned long addr,
+			void *data)
+{
+	pgprot_t prot = (pgprot_t)data;
+
+	return __change_memory(ptep, token, addr, prot, false);
+}
+
+static int change_memory_common(unsigned long addr, int numpages,
+				pgprot_t prot, bool set)
+{
+	unsigned long start = addr;
+	unsigned long size = PAGE_SIZE*numpages;
+	unsigned long end = start + size;
+	int ret;
+
+	if (start < MODULES_VADDR || start >= MODULES_END)
+		return -EINVAL;
+
+	if (end < MODULES_VADDR || end >= MODULES_END)
+		return -EINVAL;
+
+	if (set)
+		ret = apply_to_page_range(&init_mm, start, size,
+					set_page_range, (void *)prot);
+	else
+		ret = apply_to_page_range(&init_mm, start, size,
+					clear_page_range, (void *)prot);
+
+	flush_tlb_kernel_range(start, end);
+	isb();
+	return ret;
+}
+
+static int change_memory_set_bit(unsigned long addr, int numpages,
+					pgprot_t prot)
+{
+	return change_memory_common(addr, numpages, prot, true);
+}
+
+static int change_memory_clear_bit(unsigned long addr, int numpages,
+					pgprot_t prot)
+{
+	return change_memory_common(addr, numpages, prot, false);
+}
+
+int set_memory_ro(unsigned long addr, int numpages)
+{
+	return change_memory_set_bit(addr, numpages, __pgprot(PTE_RDONLY));
+}
+EXPORT_SYMBOL_GPL(set_memory_ro);
+
+int set_memory_rw(unsigned long addr, int numpages)
+{
+	return change_memory_clear_bit(addr, numpages, __pgprot(PTE_RDONLY));
+}
+EXPORT_SYMBOL_GPL(set_memory_rw);
+
+int set_memory_nx(unsigned long addr, int numpages)
+{
+	return change_memory_set_bit(addr, numpages, __pgprot(PTE_PXN));
+}
+EXPORT_SYMBOL_GPL(set_memory_nx);
+
+int set_memory_x(unsigned long addr, int numpages)
+{
+	return change_memory_clear_bit(addr, numpages, __pgprot(PTE_PXN));
+}
+EXPORT_SYMBOL_GPL(set_memory_x);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCHv2 1/4] arm64: Add CONFIG_DEBUG_SET_MODULE_RONX support

[Will Deacon]

Hi Laura,

This is looking better, but comments inline.

On Mon, Jun 02, 2014 at 09:57:35PM +0100, Laura Abbott wrote:
> 
> In a similar fashion to other architecture, add the infrastructure
> and Kconfig to enable DEBUG_SET_MODULE_RONX support. When
> enabled, module ranges will be marked read-only/no-execute as
> appropriate.
> 
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
>  arch/arm64/Kconfig.debug            |  11 ++++
>  arch/arm64/include/asm/cacheflush.h |   4 ++
>  arch/arm64/mm/Makefile              |   2 +-
>  arch/arm64/mm/pageattr.c            | 121 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 137 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/mm/pageattr.c

[...]

>  obj-$(CONFIG_HUGETLB_PAGE)	+= hugetlbpage.o
> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> new file mode 100644
> index 0000000..d8ab747
> --- /dev/null
> +++ b/arch/arm64/mm/pageattr.c
> @@ -0,0 +1,121 @@
> +/*
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/sched.h>
> +
> +#include <asm/pgtable.h>
> +#include <asm/tlbflush.h>
> +
> +static pte_t clear_pte_bit(pte_t pte, pgprot_t prot)
> +{
> +	pte_val(pte) &= ~pgprot_val(prot);
> +	return pte;
> +}
> +
> +static pte_t set_pte_bit(pte_t pte, pgprot_t prot)
> +{
> +	pte_val(pte) |= pgprot_val(prot);
> +	return pte;
> +}

We could actually re-use these for building our pte_mk* functions in
pgtable.h. Care to move them there?

> +static int __change_memory(pte_t *ptep, pgtable_t token, unsigned long addr,
> +			pgprot_t prot, bool set)
> +{
> +	pte_t pte;
> +
> +	if (set)
> +		pte = set_pte_bit(*ptep, prot);
> +	else
> +		pte = clear_pte_bit(*ptep, prot);
> +	set_pte(ptep, pte);
> +	return 0;
> +}
> +
> +static int set_page_range(pte_t *ptep, pgtable_t token, unsigned long addr,
> +			void *data)
> +{
> +	pgprot_t prot = (pgprot_t)data;
> +
> +	return __change_memory(ptep, token, addr, prot, true);
> +}
> +
> +static int clear_page_range(pte_t *ptep, pgtable_t token, unsigned long addr,
> +			void *data)
> +{
> +	pgprot_t prot = (pgprot_t)data;
> +
> +	return __change_memory(ptep, token, addr, prot, false);
> +}
> +
> +static int change_memory_common(unsigned long addr, int numpages,
> +				pgprot_t prot, bool set)
> +{
> +	unsigned long start = addr;
> +	unsigned long size = PAGE_SIZE*numpages;
> +	unsigned long end = start + size;
> +	int ret;
> +
> +	if (start < MODULES_VADDR || start >= MODULES_END)
> +		return -EINVAL;
> +
> +	if (end < MODULES_VADDR || end >= MODULES_END)
> +		return -EINVAL;

Can you use is_module_address here, or do you need to change the page
attributes for areas where no modules are currently loaded too?

> +	if (set)
> +		ret = apply_to_page_range(&init_mm, start, size,
> +					set_page_range, (void *)prot);
> +	else
> +		ret = apply_to_page_range(&init_mm, start, size,
> +					clear_page_range, (void *)prot);
> +
> +	flush_tlb_kernel_range(start, end);
> +	isb();
> +	return ret;

We already have an isb in flush_tlb_kernel_range.

> +static int change_memory_set_bit(unsigned long addr, int numpages,
> +					pgprot_t prot)
> +{
> +	return change_memory_common(addr, numpages, prot, true);
> +}
> +
> +static int change_memory_clear_bit(unsigned long addr, int numpages,
> +					pgprot_t prot)
> +{
> +	return change_memory_common(addr, numpages, prot, false);
> +}
> +
> +int set_memory_ro(unsigned long addr, int numpages)
> +{
> +	return change_memory_set_bit(addr, numpages, __pgprot(PTE_RDONLY));
> +}
> +EXPORT_SYMBOL_GPL(set_memory_ro);
> +
> +int set_memory_rw(unsigned long addr, int numpages)
> +{
> +	return change_memory_clear_bit(addr, numpages, __pgprot(PTE_RDONLY));
> +}
> +EXPORT_SYMBOL_GPL(set_memory_rw);

I'm slightly worried about the interaction with this and PTE_WRITE (see
linux-next). If the kernel pages are marked as PTE_DIRTY | PTE_WRITE, then
setting read-only is a weird contradiction. Can you take PTE_WRITE into
account for these two please?

Will

[PATCHv2 1/4] arm64: Add CONFIG_DEBUG_SET_MODULE_RONX support

[Steve Capper]

On 3 June 2014 16:22, Will Deacon <will.deacon@arm.com> wrote:

[ ... ]

>
> We already have an isb in flush_tlb_kernel_range.

Hi Will,
The following thread discusses the removal of the isb() from
flush_tlb_kernel_range:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252829.html

Also, I asked for it to be added to this series in this email thread
but you thought it was benign:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252869.html
So we could probably do without the isb(.).

Cheers,
--
Steve

[PATCHv2 1/4] arm64: Add CONFIG_DEBUG_SET_MODULE_RONX support

[Will Deacon]

On Tue, Jun 03, 2014 at 04:31:57PM +0100, Steve Capper wrote:
> On 3 June 2014 16:22, Will Deacon <will.deacon@arm.com> wrote:
> 
> [ ... ]
> 
> >
> > We already have an isb in flush_tlb_kernel_range.
> 
> Hi Will,
> The following thread discusses the removal of the isb() from
> flush_tlb_kernel_range:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252829.html
> 
> Also, I asked for it to be added to this series in this email thread
> but you thought it was benign:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252869.html
> So we could probably do without the isb(.).

Bah, I'd completely forgotten all of that! Anyway, I'd say it's up to
the patch removing the existing isb from flush_tlb_kernel_range to fix
the callers, not for other people to anticipate that change.

Even with that change, the isb isn't needed, as mentioned in the second
link above.

Will

[PATCHv2 1/4] arm64: Add CONFIG_DEBUG_SET_MODULE_RONX support

[Steve Capper]

On 3 June 2014 16:37, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jun 03, 2014 at 04:31:57PM +0100, Steve Capper wrote:
>> On 3 June 2014 16:22, Will Deacon <will.deacon@arm.com> wrote:
>>
>> [ ... ]
>>
>> >
>> > We already have an isb in flush_tlb_kernel_range.
>>
>> Hi Will,
>> The following thread discusses the removal of the isb() from
>> flush_tlb_kernel_range:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252829.html
>>
>> Also, I asked for it to be added to this series in this email thread
>> but you thought it was benign:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252869.html
>> So we could probably do without the isb(.).
>
> Bah, I'd completely forgotten all of that! Anyway, I'd say it's up to
> the patch removing the existing isb from flush_tlb_kernel_range to fix
> the callers, not for other people to anticipate that change.

Agreed.... but Catalin has applied my patch before this version hit the list:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/254828.html

>
> Even with that change, the isb isn't needed, as mentioned in the second
> link above.

Agreed :-).

[PATCHv2 1/4] arm64: Add CONFIG_DEBUG_SET_MODULE_RONX support

[Laura Abbott]

On 6/3/2014 8:22 AM, Will Deacon wrote:
> Hi Laura,
> 
> This is looking better, but comments inline.
> 
> On Mon, Jun 02, 2014 at 09:57:35PM +0100, Laura Abbott wrote:
>>
>> In a similar fashion to other architecture, add the infrastructure
>> and Kconfig to enable DEBUG_SET_MODULE_RONX support. When
>> enabled, module ranges will be marked read-only/no-execute as
>> appropriate.
>>
>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
>> ---
>>  arch/arm64/Kconfig.debug            |  11 ++++
>>  arch/arm64/include/asm/cacheflush.h |   4 ++
>>  arch/arm64/mm/Makefile              |   2 +-
>>  arch/arm64/mm/pageattr.c            | 121 ++++++++++++++++++++++++++++++++++++
>>  4 files changed, 137 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/arm64/mm/pageattr.c
> 
> [...]
> 
>>  obj-$(CONFIG_HUGETLB_PAGE)	+= hugetlbpage.o
>> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
>> new file mode 100644
>> index 0000000..d8ab747
>> --- /dev/null
>> +++ b/arch/arm64/mm/pageattr.c
>> @@ -0,0 +1,121 @@
>> +/*
>> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <linux/kernel.h>
>> +#include <linux/mm.h>
>> +#include <linux/sched.h>
>> +
>> +#include <asm/pgtable.h>
>> +#include <asm/tlbflush.h>
>> +
>> +static pte_t clear_pte_bit(pte_t pte, pgprot_t prot)
>> +{
>> +	pte_val(pte) &= ~pgprot_val(prot);
>> +	return pte;
>> +}
>> +
>> +static pte_t set_pte_bit(pte_t pte, pgprot_t prot)
>> +{
>> +	pte_val(pte) |= pgprot_val(prot);
>> +	return pte;
>> +}
> 
> We could actually re-use these for building our pte_mk* functions in
> pgtable.h. Care to move them there?
> 

Fine.

>> +static int __change_memory(pte_t *ptep, pgtable_t token, unsigned long addr,
>> +			pgprot_t prot, bool set)
>> +{
>> +	pte_t pte;
>> +
>> +	if (set)
>> +		pte = set_pte_bit(*ptep, prot);
>> +	else
>> +		pte = clear_pte_bit(*ptep, prot);
>> +	set_pte(ptep, pte);
>> +	return 0;
>> +}
>> +
>> +static int set_page_range(pte_t *ptep, pgtable_t token, unsigned long addr,
>> +			void *data)
>> +{
>> +	pgprot_t prot = (pgprot_t)data;
>> +
>> +	return __change_memory(ptep, token, addr, prot, true);
>> +}
>> +
>> +static int clear_page_range(pte_t *ptep, pgtable_t token, unsigned long addr,
>> +			void *data)
>> +{
>> +	pgprot_t prot = (pgprot_t)data;
>> +
>> +	return __change_memory(ptep, token, addr, prot, false);
>> +}
>> +
>> +static int change_memory_common(unsigned long addr, int numpages,
>> +				pgprot_t prot, bool set)
>> +{
>> +	unsigned long start = addr;
>> +	unsigned long size = PAGE_SIZE*numpages;
>> +	unsigned long end = start + size;
>> +	int ret;
>> +
>> +	if (start < MODULES_VADDR || start >= MODULES_END)
>> +		return -EINVAL;
>> +
>> +	if (end < MODULES_VADDR || end >= MODULES_END)
>> +		return -EINVAL;
> 
> Can you use is_module_address here, or do you need to change the page
> attributes for areas where no modules are currently loaded too?
> 

Yes, I think is_module_address should work fine.

>> +	if (set)
>> +		ret = apply_to_page_range(&init_mm, start, size,
>> +					set_page_range, (void *)prot);
>> +	else
>> +		ret = apply_to_page_range(&init_mm, start, size,
>> +					clear_page_range, (void *)prot);
>> +
>> +	flush_tlb_kernel_range(start, end);
>> +	isb();
>> +	return ret;
> 
> We already have an isb in flush_tlb_kernel_range.
> 

Yes, I'll drop the isb here.

>> +static int change_memory_set_bit(unsigned long addr, int numpages,
>> +					pgprot_t prot)
>> +{
>> +	return change_memory_common(addr, numpages, prot, true);
>> +}
>> +
>> +static int change_memory_clear_bit(unsigned long addr, int numpages,
>> +					pgprot_t prot)
>> +{
>> +	return change_memory_common(addr, numpages, prot, false);
>> +}
>> +
>> +int set_memory_ro(unsigned long addr, int numpages)
>> +{
>> +	return change_memory_set_bit(addr, numpages, __pgprot(PTE_RDONLY));
>> +}
>> +EXPORT_SYMBOL_GPL(set_memory_ro);
>> +
>> +int set_memory_rw(unsigned long addr, int numpages)
>> +{
>> +	return change_memory_clear_bit(addr, numpages, __pgprot(PTE_RDONLY));
>> +}
>> +EXPORT_SYMBOL_GPL(set_memory_rw);
> 
> I'm slightly worried about the interaction with this and PTE_WRITE (see
> linux-next). If the kernel pages are marked as PTE_DIRTY | PTE_WRITE, then
> setting read-only is a weird contradiction. Can you take PTE_WRITE into
> account for these two please?
>

It sounds like the solution should be to set/clear PTE_WRITE as appropriate
here, is my understanding correct?

> Will
> 

Thanks,
Laura

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

[PATCHv2 1/4] arm64: Add CONFIG_DEBUG_SET_MODULE_RONX support

[Will Deacon]

On Wed, Jun 04, 2014 at 01:56:34AM +0100, Laura Abbott wrote:
> On 6/3/2014 8:22 AM, Will Deacon wrote:
> >> +int set_memory_ro(unsigned long addr, int numpages)
> >> +{
> >> +	return change_memory_set_bit(addr, numpages, __pgprot(PTE_RDONLY));
> >> +}
> >> +EXPORT_SYMBOL_GPL(set_memory_ro);
> >> +
> >> +int set_memory_rw(unsigned long addr, int numpages)
> >> +{
> >> +	return change_memory_clear_bit(addr, numpages, __pgprot(PTE_RDONLY));
> >> +}
> >> +EXPORT_SYMBOL_GPL(set_memory_rw);
> > 
> > I'm slightly worried about the interaction with this and PTE_WRITE (see
> > linux-next). If the kernel pages are marked as PTE_DIRTY | PTE_WRITE, then
> > setting read-only is a weird contradiction. Can you take PTE_WRITE into
> > account for these two please?
> >
> 
> It sounds like the solution should be to set/clear PTE_WRITE as appropriate
> here, is my understanding correct?

You got it! You can look at set_pte_at if you're unsure (bearing in mind
that kernel mappings are always dirty).

Will

[PATCHv2 2/4] arm64: Treat handle_arch_irq as a function pointer

[Laura Abbott]

handle_arch_irq isn't actually text, it's just a function
pointer. It doesn't need to be stored in the text section
and doing so causes problems if we ever want to make the
kernel text readonly. Move it to the data section.

Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 arch/arm64/kernel/entry.S | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 39ac630..7c8ea1f 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -139,7 +139,8 @@ tsk	.req	x28		// current thread_info
  * Interrupt handling.
  */
 	.macro	irq_handler
-	ldr	x1, handle_arch_irq
+	ldr	x1, =handle_arch_irq
+	ldr	x1, [x1]
 	mov	x0, sp
 	blr	x1
 	.endm
@@ -678,5 +679,8 @@ ENTRY(sys_rt_sigreturn_wrapper)
 	b	sys_rt_sigreturn
 ENDPROC(sys_rt_sigreturn_wrapper)
 
-ENTRY(handle_arch_irq)
+	.data
+	.globl handle_arch_irq
+	.align 4
+handle_arch_irq:
 	.quad	0
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCHv2 2/4] arm64: Treat handle_arch_irq as a function pointer

[Mark Rutland]

Hi Laura,

On Mon, Jun 02, 2014 at 09:57:36PM +0100, Laura Abbott wrote:
> 
> handle_arch_irq isn't actually text, it's just a function
> pointer. It doesn't need to be stored in the text section
> and doing so causes problems if we ever want to make the
> kernel text readonly. Move it to the data section.
> 
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
>  arch/arm64/kernel/entry.S | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 39ac630..7c8ea1f 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -139,7 +139,8 @@ tsk	.req	x28		// current thread_info
>   * Interrupt handling.
>   */
>  	.macro	irq_handler
> -	ldr	x1, handle_arch_irq
> +	ldr	x1, =handle_arch_irq
> +	ldr	x1, [x1]
>  	mov	x0, sp
>  	blr	x1
>  	.endm
> @@ -678,5 +679,8 @@ ENTRY(sys_rt_sigreturn_wrapper)
>  	b	sys_rt_sigreturn
>  ENDPROC(sys_rt_sigreturn_wrapper)
>  
> -ENTRY(handle_arch_irq)
> +	.data
> +	.globl handle_arch_irq
> +	.align 4
> +handle_arch_irq:
>  	.quad	0

Rather than having to set the alignment and section manually, would
something like the below work, moving the definition into kernel/irq.c?

Cheers,
Mark.

---->8----
diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index e1f7ecd..1eebf5b 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -3,7 +3,6 @@
 
 #include <asm-generic/irq.h>
 
-extern void (*handle_arch_irq)(struct pt_regs *);
 extern void migrate_irqs(void);
 extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
 
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 39ac630..d378fc0 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -139,7 +139,8 @@ tsk .req    x28             // current thread_info
  * Interrupt handling.
  */
        .macro  irq_handler
-       ldr     x1, handle_arch_irq
+       ldr     x1, =handle_arch_irq
+       ldr     x1, [x1]
        mov     x0, sp
        blr     x1
        .endm
@@ -677,6 +678,3 @@ ENTRY(sys_rt_sigreturn_wrapper)
        mov     x0, sp
        b       sys_rt_sigreturn
 ENDPROC(sys_rt_sigreturn_wrapper)
-
-ENTRY(handle_arch_irq)
-       .quad   0
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..e4fedbc 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -67,6 +67,8 @@ void handle_IRQ(unsigned int irq, struct pt_regs *regs)
        set_irq_regs(old_regs);
 }
 
+void (*handle_arch_irq)(struct pt_regs *) = NULL;
+
 void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
 {
        if (handle_arch_irq)

[PATCHv2 2/4] arm64: Treat handle_arch_irq as a function pointer

[Ard Biesheuvel]

On 3 June 2014 11:13, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Laura,
>
> On Mon, Jun 02, 2014 at 09:57:36PM +0100, Laura Abbott wrote:
>>
>> handle_arch_irq isn't actually text, it's just a function
>> pointer. It doesn't need to be stored in the text section
>> and doing so causes problems if we ever want to make the
>> kernel text readonly. Move it to the data section.
>>
>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
>> ---
>>  arch/arm64/kernel/entry.S | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 39ac630..7c8ea1f 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -139,7 +139,8 @@ tsk       .req    x28             // current thread_info
>>   * Interrupt handling.
>>   */
>>       .macro  irq_handler
>> -     ldr     x1, handle_arch_irq
>> +     ldr     x1, =handle_arch_irq
>> +     ldr     x1, [x1]
>>       mov     x0, sp
>>       blr     x1
>>       .endm
>> @@ -678,5 +679,8 @@ ENTRY(sys_rt_sigreturn_wrapper)
>>       b       sys_rt_sigreturn
>>  ENDPROC(sys_rt_sigreturn_wrapper)
>>
>> -ENTRY(handle_arch_irq)
>> +     .data
>> +     .globl handle_arch_irq
>> +     .align 4
>> +handle_arch_irq:
>>       .quad   0
>
> Rather than having to set the alignment and section manually, would
> something like the below work, moving the definition into kernel/irq.c?
>
> Cheers,
> Mark.
>
> ---->8----
> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
> index e1f7ecd..1eebf5b 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -3,7 +3,6 @@
>u
>  #include <asm-generic/irq.h>
>
> -extern void (*handle_arch_irq)(struct pt_regs *);
>  extern void migrate_irqs(void);
>  extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
>
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 39ac630..d378fc0 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -139,7 +139,8 @@ tsk .req    x28             // current thread_info
>   * Interrupt handling.
>   */
>         .macro  irq_handler
> -       ldr     x1, handle_arch_irq
> +       ldr     x1, =handle_arch_irq
> +       ldr     x1, [x1]

How about using

adrp x1, handle_arch_irq
ldr x1, [x1, #:lo12:handle_arch_irq]

instead, and avoid the literal pool entry?

-- 
Ard.


>         mov     x0, sp
>         blr     x1
>         .endm
> @@ -677,6 +678,3 @@ ENTRY(sys_rt_sigreturn_wrapper)
>         mov     x0, sp
>         b       sys_rt_sigreturn
>  ENDPROC(sys_rt_sigreturn_wrapper)
> -
> -ENTRY(handle_arch_irq)
> -       .quad   0
> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> index 0f08dfd..e4fedbc 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -67,6 +67,8 @@ void handle_IRQ(unsigned int irq, struct pt_regs *regs)
>         set_irq_regs(old_regs);
>  }
>
> +void (*handle_arch_irq)(struct pt_regs *) = NULL;
> +
>  void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
>  {
>         if (handle_arch_irq)
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2 3/4] arm64: Switch to ldr for loading the stub vectors

[Laura Abbott]

The hyp stub vectors are currently loaded using adr. This
instruction has a +/- 1MB range for the loading address. If
the alignment for sections is changed. the address may be more
than 1MB away, resulting in reclocation errors. Switch to using
ldr for getting the address to ensure we aren't affected by the
location of the __hyp_stub_vectors.

Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 arch/arm64/kernel/head.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 0fd5650..07574f4 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -208,7 +208,7 @@ CPU_LE(	movk	x0, #0x30d0, lsl #16	)	// Clear EE and E0E on LE systems
 	msr	vttbr_el2, xzr
 
 	/* Hypervisor stub */
-	adr	x0, __hyp_stub_vectors
+	ldr	x0, =__hyp_stub_vectors
 	msr	vbar_el2, x0
 
 	/* spsr */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCHv2 3/4] arm64: Switch to ldr for loading the stub vectors

[Will Deacon]

On Mon, Jun 02, 2014 at 09:57:37PM +0100, Laura Abbott wrote:
> 
> The hyp stub vectors are currently loaded using adr. This
> instruction has a +/- 1MB range for the loading address. If
> the alignment for sections is changed. the address may be more
> than 1MB away, resulting in reclocation errors. Switch to using
> ldr for getting the address to ensure we aren't affected by the
> location of the __hyp_stub_vectors.
> 
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
>  arch/arm64/kernel/head.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 0fd5650..07574f4 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -208,7 +208,7 @@ CPU_LE(	movk	x0, #0x30d0, lsl #16	)	// Clear EE and E0E on LE systems
>  	msr	vttbr_el2, xzr
>  
>  	/* Hypervisor stub */
> -	adr	x0, __hyp_stub_vectors
> +	ldr	x0, =__hyp_stub_vectors
>  	msr	vbar_el2, x0

Or use Ard's trick with adrp and the lo12 relocation?

Will

[PATCHv2 4/4] arm64: add better page protections to arm64

[Laura Abbott]

Add page protections for arm64 similar to those in arm or in
progress for arm. This is for security reasons. The flow is
currently:

- Map all memory as either RWX or RW. We round to the nearest
  section to avoid creating page tables before everything is mapped
- Once everything is mapped, if either end of the RWX section should
  not be X, we split the PMD and remap as necessary
- When initmem is to be freed, we change the permissions back to
  RW (using stop machine if necessary to flush the TLB)
- If CONFIG_DEBUG_RODATA is set, the read only sections are set
  read only.

Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 arch/arm64/Kconfig.debug        |  23 ++++++
 arch/arm64/kernel/vmlinux.lds.S |  17 ++++
 arch/arm64/mm/init.c            |   1 +
 arch/arm64/mm/mm.h              |   2 +
 arch/arm64/mm/mmu.c             | 173 ++++++++++++++++++++++++++++++++++++----
 5 files changed, 200 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index 53979ac..f51f3af 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -48,4 +48,27 @@ config DEBUG_SET_MODULE_RONX
           against certain classes of kernel exploits.
           If in doubt, say "N".
 
+config DEBUG_RODATA
+	bool "Make kernel text and rodata read-only"
+	help
+	  If this is set, kernel text and rodata will be made read-only. This
+	  is to help catch accidental or malicious attempts to change the
+	  kernel's executable code. Additionally splits rodata from kernel
+	  text so it can be made explicitly non-executable.
+
+          If in doubt, say Y
+
+config DEBUG_ALIGN_RODATA
+	depends on DEBUG_RODATA
+	bool "Align linker sections up to SECTION_SIZE"
+	help
+	  If this option is enabled, sections that may potentially be marked as
+	  read only or non-executable will be aligned up to the section size of
+	  the kernel. This prevents sections from being split into pages and
+	  avoids a potential TLB penalty. The downside is an increase in
+	  alignment and potentially wasted space. Turn on this option if
+	  performance is more important than memory pressure.
+
+	  If in doubt, say N
+
 endmenu
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 4ba7a55..7643ae6 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -8,6 +8,7 @@
 #include <asm/thread_info.h>
 #include <asm/memory.h>
 #include <asm/page.h>
+#include <asm/pgtable.h>
 
 #define ARM_EXIT_KEEP(x)
 #define ARM_EXIT_DISCARD(x)	x
@@ -52,6 +53,9 @@ SECTIONS
 		_text = .;
 		HEAD_TEXT
 	}
+#ifdef DEBUG_ALIGN_RODATA
+	. = ALIGN(1<<SECTION_SHIFT);
+#endif
 	.text : {			/* Real text segment		*/
 		_stext = .;		/* Text and read-only data	*/
 			__exception_text_start = .;
@@ -68,19 +72,32 @@ SECTIONS
 		*(.got)			/* Global offset table		*/
 	}
 
+#ifdef DEBUG_ALIGN_RODATA
+	. = ALIGN(1<<SECTION_SHIFT);
+#endif
 	RO_DATA(PAGE_SIZE)
 	EXCEPTION_TABLE(8)
 	NOTES
 	_etext = .;			/* End of text and rodata section */
 
+#ifdef DEBUG_ALIGN_RODATA
+	. = ALIGN(1<<SECTION_SHIFT);
+#else
 	. = ALIGN(PAGE_SIZE);
+#endif
 	__init_begin = .;
 
 	INIT_TEXT_SECTION(8)
 	.exit.text : {
 		ARM_EXIT_KEEP(EXIT_TEXT)
 	}
+
+#ifdef DEBUG_ALIGN_RODATA
+	. = ALIGN(1<<SECTION_SHIFT);
+	__init_data_begin = .;
+#else
 	. = ALIGN(16);
+#endif
 	.init.data : {
 		INIT_DATA
 		INIT_SETUP(16)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 51d5352..bc74a3a 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -325,6 +325,7 @@ void __init mem_init(void)
 
 void free_initmem(void)
 {
+	fixup_init();
 	free_initmem_default(0);
 }
 
diff --git a/arch/arm64/mm/mm.h b/arch/arm64/mm/mm.h
index d519f4f..82347d8 100644
--- a/arch/arm64/mm/mm.h
+++ b/arch/arm64/mm/mm.h
@@ -1,2 +1,4 @@
 extern void __init bootmem_init(void);
 extern void __init arm64_swiotlb_init(void);
+
+void fixup_init(void);
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 0a472c4..1300886 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -26,6 +26,7 @@
 #include <linux/memblock.h>
 #include <linux/fs.h>
 #include <linux/io.h>
+#include <linux/stop_machine.h>
 
 #include <asm/cputype.h>
 #include <asm/sections.h>
@@ -167,26 +168,67 @@ static void __init *early_alloc(unsigned long sz)
 	return ptr;
 }
 
-static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
-				  unsigned long end, unsigned long pfn)
+/*
+ * remap a PMD into pages
+ */
+static noinline void __ref split_pmd(pmd_t *pmd, pgprot_t prot, bool early)
+{
+	pte_t *pte, *start_pte;
+	u64 val;
+	unsigned long pfn;
+	int i = 0;
+
+	val = pmd_val(*pmd);
+
+	if (early)
+		start_pte = pte = early_alloc(PTRS_PER_PTE*sizeof(pte_t));
+	else
+		start_pte = pte = (pte_t *)__get_free_page(PGALLOC_GFP);
+
+	BUG_ON(!pte);
+
+
+	pfn = __phys_to_pfn(val & PHYS_MASK);
+
+	do {
+		set_pte(pte, pfn_pte(pfn, PAGE_KERNEL_EXEC));
+		pfn++;
+	} while (pte++, i++, i < PTRS_PER_PTE);
+
+
+	__pmd_populate(pmd, __pa(start_pte), PMD_TYPE_TABLE);
+	flush_tlb_all();
+}
+
+static void __ref alloc_init_pte(pmd_t *pmd, unsigned long addr,
+				  unsigned long end, unsigned long pfn,
+				  pgprot_t prot, bool early)
 {
 	pte_t *pte;
 
 	if (pmd_none(*pmd)) {
-		pte = early_alloc(PTRS_PER_PTE * sizeof(pte_t));
+		if (early)
+			pte = early_alloc(PTRS_PER_PTE * sizeof(pte_t));
+		else
+			pte = (pte_t *)__get_free_page(PGALLOC_GFP);
+		BUG_ON(!pte);
 		__pmd_populate(pmd, __pa(pte), PMD_TYPE_TABLE);
 	}
-	BUG_ON(pmd_bad(*pmd));
+
+	if (pmd_bad(*pmd))
+		split_pmd(pmd, prot, early);
 
 	pte = pte_offset_kernel(pmd, addr);
 	do {
-		set_pte(pte, pfn_pte(pfn, PAGE_KERNEL_EXEC));
+		set_pte(pte, pfn_pte(pfn, prot));
 		pfn++;
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 }
 
-static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
-				  unsigned long end, phys_addr_t phys)
+static void __ref alloc_init_pmd(pud_t *pud, unsigned long addr,
+				  unsigned long end, phys_addr_t phys,
+				  pgprot_t sect_prot, pgprot_t pte_prot,
+				  bool early)
 {
 	pmd_t *pmd;
 	unsigned long next;
@@ -195,7 +237,11 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
 	 * Check for initial section mappings in the pgd/pud and remove them.
 	 */
 	if (pud_none(*pud) || pud_bad(*pud)) {
-		pmd = early_alloc(PTRS_PER_PMD * sizeof(pmd_t));
+		if (early)
+			pmd = early_alloc(PTRS_PER_PMD * sizeof(pmd_t));
+		else
+			pmd = pmd_alloc_one(&init_mm, addr);
+		BUG_ON(!pmd);
 		pud_populate(&init_mm, pud, pmd);
 	}
 
@@ -213,21 +259,25 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
 			if (!pmd_none(old_pmd))
 				flush_tlb_all();
 		} else {
-			alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys));
+			alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys),
+					pte_prot, early);
 		}
 		phys += next - addr;
 	} while (pmd++, addr = next, addr != end);
 }
 
-static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,
-				  unsigned long end, unsigned long phys)
+static void __ref alloc_init_pud(pgd_t *pgd, unsigned long addr,
+				  unsigned long end, unsigned long phys,
+				  pgprot_t sect_prot, pgprot_t pte_prot,
+				  bool early)
 {
 	pud_t *pud = pud_offset(pgd, addr);
 	unsigned long next;
 
 	do {
 		next = pud_addr_end(addr, end);
-		alloc_init_pmd(pud, addr, next, phys);
+		alloc_init_pmd(pud, addr, next, phys, sect_prot, pte_prot,
+				early);
 		phys += next - addr;
 	} while (pud++, addr = next, addr != end);
 }
@@ -236,8 +286,10 @@ static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,
  * Create the page directory entries and any necessary page tables for the
  * mapping specified by 'md'.
  */
-static void __init create_mapping(phys_addr_t phys, unsigned long virt,
-				  phys_addr_t size)
+static void __ref __create_mapping(phys_addr_t phys, unsigned long virt,
+				  phys_addr_t size,
+				  pgprot_t sect_prot, pgprot_t pte_prot,
+				  bool early)
 {
 	unsigned long addr, length, end, next;
 	pgd_t *pgd;
@@ -255,15 +307,37 @@ static void __init create_mapping(phys_addr_t phys, unsigned long virt,
 	end = addr + length;
 	do {
 		next = pgd_addr_end(addr, end);
-		alloc_init_pud(pgd, addr, next, phys);
+		alloc_init_pud(pgd, addr, next, phys, sect_prot, pte_prot,
+				early);
 		phys += next - addr;
 	} while (pgd++, addr = next, addr != end);
 }
 
+static void __ref create_mapping(phys_addr_t phys, unsigned long virt,
+				  phys_addr_t size,
+				  pgprot_t sect_prot, pgprot_t pte_prot)
+{
+	return __create_mapping(phys, virt, size, sect_prot, pte_prot, true);
+}
+
+static void __ref create_mapping_late(phys_addr_t phys, unsigned long virt,
+				  phys_addr_t size,
+				  pgprot_t sect_prot, pgprot_t pte_prot)
+{
+	return __create_mapping(phys, virt, size, sect_prot, pte_prot, false);
+}
+
 static void __init map_mem(void)
 {
 	struct memblock_region *reg;
 	phys_addr_t limit;
+	/*
+	 * Set up the executable regions using the exising section mappings
+	 * foir now. This will get more fine grained later once all memory
+	 * is mapped
+	 */
+	unsigned long kernel_x_start = round_down(__pa(_stext), SECTION_SIZE);
+	unsigned long kernel_x_end = round_up(__pa(__init_end), SECTION_SIZE);
 
 	/*
 	 * Temporarily limit the memblock range. We need to do this as
@@ -301,13 +375,79 @@ static void __init map_mem(void)
 		}
 #endif
 
-		create_mapping(start, __phys_to_virt(start), end - start);
+		if (end < kernel_x_start) {
+			create_mapping(start, __phys_to_virt(start), end - start,
+				prot_sect_kernel, pgprot_default);
+		} else if (start >= kernel_x_end) {
+			create_mapping(start, __phys_to_virt(start), end - start,
+				prot_sect_kernel | PMD_SECT_PXN, pgprot_default | PTE_PXN);
+		} else {
+			if (start < kernel_x_start)
+				create_mapping(start, __phys_to_virt(start), kernel_x_start - start,
+					prot_sect_kernel | PMD_SECT_PXN, pgprot_default | PTE_PXN);
+			create_mapping(kernel_x_start, __phys_to_virt(kernel_x_start), kernel_x_end - kernel_x_start,
+				prot_sect_kernel, pgprot_default);
+			if (kernel_x_end < end)
+				create_mapping(kernel_x_end, __phys_to_virt(kernel_x_end), end - kernel_x_end,
+					prot_sect_kernel | PMD_SECT_PXN, pgprot_default | PTE_PXN);
+
+
+		}
+
 	}
 
 	/* Limit no longer required. */
 	memblock_set_current_limit(MEMBLOCK_ALLOC_ANYWHERE);
 }
 
+void __init fixup_executable(void)
+{
+	/* now that we are actually fully mapped, make the start/end more fine grained */
+	if (!IS_ALIGNED((unsigned long)_stext, SECTION_SIZE)) {
+		unsigned long aligned_start = round_down(__pa(_stext), SECTION_SIZE);
+
+		create_mapping(aligned_start, __phys_to_virt(aligned_start),
+				__pa(_stext) - aligned_start,
+				prot_sect_kernel | PMD_SECT_PXN,
+				pgprot_default | PTE_PXN);
+	}
+
+	if (!IS_ALIGNED((unsigned long)__init_end, SECTION_SIZE)) {
+		unsigned long aligned_end = round_up(__pa(__init_end), SECTION_SIZE);
+		create_mapping(__pa(__init_end), (unsigned long)__init_end,
+				aligned_end - __pa(__init_end),
+				prot_sect_kernel | PMD_SECT_PXN, pgprot_default | PTE_PXN);
+	}
+}
+
+#ifdef CONFIG_DEBUG_RODATA
+void mark_rodata_ro(void)
+{
+	create_mapping_late(__pa(_stext), (unsigned long)_stext, (unsigned long)_etext - (unsigned long)_stext,
+				prot_sect_kernel | PMD_SECT_RDONLY,
+				pgprot_default | PTE_RDONLY);
+
+}
+#endif
+
+static int __flush_mappings(void *unused)
+{
+	flush_tlb_kernel_range((unsigned long)__init_begin, (unsigned long)__init_end);
+	return 0;
+}
+
+void __ref fixup_init(void)
+{
+	phys_addr_t start = __pa(__init_begin);
+	phys_addr_t end = __pa(__init_end);
+
+	create_mapping_late(start, (unsigned long)__init_begin,
+			end - start,
+			prot_sect_kernel | PMD_SECT_PXN, pgprot_default | PTE_PXN);
+	if (!IS_ALIGNED(start, SECTION_SIZE) || !IS_ALIGNED(end, SECTION_SIZE))
+		stop_machine(__flush_mappings, NULL, NULL);
+}
+
 /*
  * paging_init() sets up the page tables, initialises the zone memory
  * maps and sets up the zero page.
@@ -317,6 +457,7 @@ void __init paging_init(void)
 	void *zero_page;
 
 	map_mem();
+	fixup_executable();
 
 	/*
 	 * Finally flush the caches and tlb to ensure that we're in a
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCHv2 4/4] arm64: add better page protections to arm64

[Steve Capper]

Hi Laura,
I have some comments below:

On 2 June 2014 21:57, Laura Abbott <lauraa@codeaurora.org> wrote:
>
> Add page protections for arm64 similar to those in arm or in
> progress for arm. This is for security reasons. The flow is
> currently:
>
> - Map all memory as either RWX or RW. We round to the nearest
>   section to avoid creating page tables before everything is mapped
> - Once everything is mapped, if either end of the RWX section should
>   not be X, we split the PMD and remap as necessary
> - When initmem is to be freed, we change the permissions back to
>   RW (using stop machine if necessary to flush the TLB)
> - If CONFIG_DEBUG_RODATA is set, the read only sections are set
>   read only.
>
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
>  arch/arm64/Kconfig.debug        |  23 ++++++
>  arch/arm64/kernel/vmlinux.lds.S |  17 ++++
>  arch/arm64/mm/init.c            |   1 +
>  arch/arm64/mm/mm.h              |   2 +
>  arch/arm64/mm/mmu.c             | 173 ++++++++++++++++++++++++++++++++++++----
>  5 files changed, 200 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index 53979ac..f51f3af 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -48,4 +48,27 @@ config DEBUG_SET_MODULE_RONX
>            against certain classes of kernel exploits.
>            If in doubt, say "N".
>
> +config DEBUG_RODATA
> +       bool "Make kernel text and rodata read-only"
> +       help
> +         If this is set, kernel text and rodata will be made read-only. This
> +         is to help catch accidental or malicious attempts to change the
> +         kernel's executable code. Additionally splits rodata from kernel
> +         text so it can be made explicitly non-executable.
> +
> +          If in doubt, say Y
> +
> +config DEBUG_ALIGN_RODATA
> +       depends on DEBUG_RODATA
> +       bool "Align linker sections up to SECTION_SIZE"
> +       help
> +         If this option is enabled, sections that may potentially be marked as
> +         read only or non-executable will be aligned up to the section size of
> +         the kernel. This prevents sections from being split into pages and
> +         avoids a potential TLB penalty. The downside is an increase in
> +         alignment and potentially wasted space. Turn on this option if
> +         performance is more important than memory pressure.
> +
> +         If in doubt, say N
> +
>  endmenu
> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index 4ba7a55..7643ae6 100644
> --- a/arch/arm64/kernel/vmlinux.lds.S
> +++ b/arch/arm64/kernel/vmlinux.lds.S
> @@ -8,6 +8,7 @@
>  #include <asm/thread_info.h>
>  #include <asm/memory.h>
>  #include <asm/page.h>
> +#include <asm/pgtable.h>
>
>  #define ARM_EXIT_KEEP(x)
>  #define ARM_EXIT_DISCARD(x)    x
> @@ -52,6 +53,9 @@ SECTIONS
>                 _text = .;
>                 HEAD_TEXT
>         }
> +#ifdef DEBUG_ALIGN_RODATA
> +       . = ALIGN(1<<SECTION_SHIFT);
> +#endif
>         .text : {                       /* Real text segment            */
>                 _stext = .;             /* Text and read-only data      */
>                         __exception_text_start = .;
> @@ -68,19 +72,32 @@ SECTIONS
>                 *(.got)                 /* Global offset table          */
>         }
>
> +#ifdef DEBUG_ALIGN_RODATA
> +       . = ALIGN(1<<SECTION_SHIFT);
> +#endif
>         RO_DATA(PAGE_SIZE)
>         EXCEPTION_TABLE(8)
>         NOTES
>         _etext = .;                     /* End of text and rodata section */
>
> +#ifdef DEBUG_ALIGN_RODATA
> +       . = ALIGN(1<<SECTION_SHIFT);
> +#else
>         . = ALIGN(PAGE_SIZE);
> +#endif
>         __init_begin = .;
>
>         INIT_TEXT_SECTION(8)
>         .exit.text : {
>                 ARM_EXIT_KEEP(EXIT_TEXT)
>         }
> +
> +#ifdef DEBUG_ALIGN_RODATA
> +       . = ALIGN(1<<SECTION_SHIFT);
> +       __init_data_begin = .;
> +#else
>         . = ALIGN(16);
> +#endif
>         .init.data : {
>                 INIT_DATA
>                 INIT_SETUP(16)
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index 51d5352..bc74a3a 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -325,6 +325,7 @@ void __init mem_init(void)
>
>  void free_initmem(void)
>  {
> +       fixup_init();
>         free_initmem_default(0);
>  }
>
> diff --git a/arch/arm64/mm/mm.h b/arch/arm64/mm/mm.h
> index d519f4f..82347d8 100644
> --- a/arch/arm64/mm/mm.h
> +++ b/arch/arm64/mm/mm.h
> @@ -1,2 +1,4 @@
>  extern void __init bootmem_init(void);
>  extern void __init arm64_swiotlb_init(void);
> +
> +void fixup_init(void);
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 0a472c4..1300886 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -26,6 +26,7 @@
>  #include <linux/memblock.h>
>  #include <linux/fs.h>
>  #include <linux/io.h>
> +#include <linux/stop_machine.h>
>
>  #include <asm/cputype.h>
>  #include <asm/sections.h>
> @@ -167,26 +168,67 @@ static void __init *early_alloc(unsigned long sz)
>         return ptr;
>  }
>
> -static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
> -                                 unsigned long end, unsigned long pfn)
> +/*
> + * remap a PMD into pages
> + */

As a heads-up, I've sent off a patch that makes use of 1GB pud
mappings for the kernel:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/253516.html

And Catalin has applied it (to his devel tree I think?):
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/254803.html

So some logic may be needed for split_pud.

> +static noinline void __ref split_pmd(pmd_t *pmd, pgprot_t prot, bool early)
> +{

prot does not appear to be used by this function?

> +       pte_t *pte, *start_pte;
> +       u64 val;
> +       unsigned long pfn;
> +       int i = 0;
> +
> +       val = pmd_val(*pmd);
> +
> +       if (early)
> +               start_pte = pte = early_alloc(PTRS_PER_PTE*sizeof(pte_t));
> +       else
> +               start_pte = pte = (pte_t *)__get_free_page(PGALLOC_GFP);
> +
> +       BUG_ON(!pte);
> +
> +
> +       pfn = __phys_to_pfn(val & PHYS_MASK);

Would it be better to have:
pfn = pmd_pfn(*pmd);

> +
> +       do {
> +               set_pte(pte, pfn_pte(pfn, PAGE_KERNEL_EXEC));
> +               pfn++;
> +       } while (pte++, i++, i < PTRS_PER_PTE);
> +
> +
> +       __pmd_populate(pmd, __pa(start_pte), PMD_TYPE_TABLE);
> +       flush_tlb_all();
> +}
> +
> +static void __ref alloc_init_pte(pmd_t *pmd, unsigned long addr,
> +                                 unsigned long end, unsigned long pfn,
> +                                 pgprot_t prot, bool early)
>  {
>         pte_t *pte;
>
>         if (pmd_none(*pmd)) {
> -               pte = early_alloc(PTRS_PER_PTE * sizeof(pte_t));
> +               if (early)
> +                       pte = early_alloc(PTRS_PER_PTE * sizeof(pte_t));
> +               else
> +                       pte = (pte_t *)__get_free_page(PGALLOC_GFP);
> +               BUG_ON(!pte);
>                 __pmd_populate(pmd, __pa(pte), PMD_TYPE_TABLE);
>         }
> -       BUG_ON(pmd_bad(*pmd));
> +
> +       if (pmd_bad(*pmd))
> +               split_pmd(pmd, prot, early);
>
>         pte = pte_offset_kernel(pmd, addr);
>         do {
> -               set_pte(pte, pfn_pte(pfn, PAGE_KERNEL_EXEC));
> +               set_pte(pte, pfn_pte(pfn, prot));
>                 pfn++;
>         } while (pte++, addr += PAGE_SIZE, addr != end);
>  }
>
> -static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
> -                                 unsigned long end, phys_addr_t phys)
> +static void __ref alloc_init_pmd(pud_t *pud, unsigned long addr,
> +                                 unsigned long end, phys_addr_t phys,
> +                                 pgprot_t sect_prot, pgprot_t pte_prot,
> +                                 bool early)
>  {
>         pmd_t *pmd;
>         unsigned long next;
> @@ -195,7 +237,11 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
>          * Check for initial section mappings in the pgd/pud and remove them.
>          */
>         if (pud_none(*pud) || pud_bad(*pud)) {
> -               pmd = early_alloc(PTRS_PER_PMD * sizeof(pmd_t));
> +               if (early)
> +                       pmd = early_alloc(PTRS_PER_PMD * sizeof(pmd_t));
> +               else
> +                       pmd = pmd_alloc_one(&init_mm, addr);
> +               BUG_ON(!pmd);
>                 pud_populate(&init_mm, pud, pmd);
>         }
>
> @@ -213,21 +259,25 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
>                         if (!pmd_none(old_pmd))
>                                 flush_tlb_all();
>                 } else {
> -                       alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys));
> +                       alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys),
> +                                       pte_prot, early);
>                 }
>                 phys += next - addr;
>         } while (pmd++, addr = next, addr != end);
>  }
>
> -static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,
> -                                 unsigned long end, unsigned long phys)
> +static void __ref alloc_init_pud(pgd_t *pgd, unsigned long addr,
> +                                 unsigned long end, unsigned long phys,
> +                                 pgprot_t sect_prot, pgprot_t pte_prot,
> +                                 bool early)
>  {
>         pud_t *pud = pud_offset(pgd, addr);
>         unsigned long next;
>
>         do {
>                 next = pud_addr_end(addr, end);
> -               alloc_init_pmd(pud, addr, next, phys);
> +               alloc_init_pmd(pud, addr, next, phys, sect_prot, pte_prot,
> +                               early);
>                 phys += next - addr;
>         } while (pud++, addr = next, addr != end);
>  }
> @@ -236,8 +286,10 @@ static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,
>   * Create the page directory entries and any necessary page tables for the
>   * mapping specified by 'md'.
>   */
> -static void __init create_mapping(phys_addr_t phys, unsigned long virt,
> -                                 phys_addr_t size)
> +static void __ref __create_mapping(phys_addr_t phys, unsigned long virt,
> +                                 phys_addr_t size,
> +                                 pgprot_t sect_prot, pgprot_t pte_prot,
> +                                 bool early)
>  {
>         unsigned long addr, length, end, next;
>         pgd_t *pgd;
> @@ -255,15 +307,37 @@ static void __init create_mapping(phys_addr_t phys, unsigned long virt,
>         end = addr + length;
>         do {
>                 next = pgd_addr_end(addr, end);
> -               alloc_init_pud(pgd, addr, next, phys);
> +               alloc_init_pud(pgd, addr, next, phys, sect_prot, pte_prot,
> +                               early);
>                 phys += next - addr;
>         } while (pgd++, addr = next, addr != end);
>  }
>
> +static void __ref create_mapping(phys_addr_t phys, unsigned long virt,
> +                                 phys_addr_t size,
> +                                 pgprot_t sect_prot, pgprot_t pte_prot)
> +{
> +       return __create_mapping(phys, virt, size, sect_prot, pte_prot, true);
> +}
> +
> +static void __ref create_mapping_late(phys_addr_t phys, unsigned long virt,
> +                                 phys_addr_t size,
> +                                 pgprot_t sect_prot, pgprot_t pte_prot)
> +{
> +       return __create_mapping(phys, virt, size, sect_prot, pte_prot, false);
> +}
> +
>  static void __init map_mem(void)
>  {
>         struct memblock_region *reg;
>         phys_addr_t limit;
> +       /*
> +        * Set up the executable regions using the exising section mappings

nit: existing

> +        * foir now. This will get more fine grained later once all memory

nit: for

> +        * is mapped
> +        */
> +       unsigned long kernel_x_start = round_down(__pa(_stext), SECTION_SIZE);
> +       unsigned long kernel_x_end = round_up(__pa(__init_end), SECTION_SIZE);

This logic is rounded to SECTION_SIZE, again a heads-up, the 1GB
mappings would benefit from some different logic.

>
>         /*
>          * Temporarily limit the memblock range. We need to do this as
> @@ -301,13 +375,79 @@ static void __init map_mem(void)
>                 }
>  #endif
>
> -               create_mapping(start, __phys_to_virt(start), end - start);
> +               if (end < kernel_x_start) {
> +                       create_mapping(start, __phys_to_virt(start), end - start,
> +                               prot_sect_kernel, pgprot_default);
> +               } else if (start >= kernel_x_end) {
> +                       create_mapping(start, __phys_to_virt(start), end - start,
> +                               prot_sect_kernel | PMD_SECT_PXN, pgprot_default | PTE_PXN);
> +               } else {
> +                       if (start < kernel_x_start)
> +                               create_mapping(start, __phys_to_virt(start), kernel_x_start - start,
> +                                       prot_sect_kernel | PMD_SECT_PXN, pgprot_default | PTE_PXN);
> +                       create_mapping(kernel_x_start, __phys_to_virt(kernel_x_start), kernel_x_end - kernel_x_start,
> +                               prot_sect_kernel, pgprot_default);
> +                       if (kernel_x_end < end)
> +                               create_mapping(kernel_x_end, __phys_to_virt(kernel_x_end), end - kernel_x_end,
> +                                       prot_sect_kernel | PMD_SECT_PXN, pgprot_default | PTE_PXN);
> +
> +
> +               }
> +

Could a config option perhaps be made available for people who are
happy to run with the simpler mappings?
create_mapping(start, __phys_to_virt(start), end - start);


>         }
>
>         /* Limit no longer required. */
>         memblock_set_current_limit(MEMBLOCK_ALLOC_ANYWHERE);
>  }
>
> +void __init fixup_executable(void)
> +{
> +       /* now that we are actually fully mapped, make the start/end more fine grained */
> +       if (!IS_ALIGNED((unsigned long)_stext, SECTION_SIZE)) {
> +               unsigned long aligned_start = round_down(__pa(_stext), SECTION_SIZE);
> +
> +               create_mapping(aligned_start, __phys_to_virt(aligned_start),
> +                               __pa(_stext) - aligned_start,
> +                               prot_sect_kernel | PMD_SECT_PXN,
> +                               pgprot_default | PTE_PXN);
> +       }
> +
> +       if (!IS_ALIGNED((unsigned long)__init_end, SECTION_SIZE)) {
> +               unsigned long aligned_end = round_up(__pa(__init_end), SECTION_SIZE);
> +               create_mapping(__pa(__init_end), (unsigned long)__init_end,
> +                               aligned_end - __pa(__init_end),
> +                               prot_sect_kernel | PMD_SECT_PXN, pgprot_default | PTE_PXN);
> +       }
> +}
> +
> +#ifdef CONFIG_DEBUG_RODATA
> +void mark_rodata_ro(void)
> +{
> +       create_mapping_late(__pa(_stext), (unsigned long)_stext, (unsigned long)_etext - (unsigned long)_stext,
> +                               prot_sect_kernel | PMD_SECT_RDONLY,
> +                               pgprot_default | PTE_RDONLY);
> +
> +}
> +#endif
> +
> +static int __flush_mappings(void *unused)
> +{
> +       flush_tlb_kernel_range((unsigned long)__init_begin, (unsigned long)__init_end);
> +       return 0;
> +}
> +
> +void __ref fixup_init(void)
> +{
> +       phys_addr_t start = __pa(__init_begin);
> +       phys_addr_t end = __pa(__init_end);
> +
> +       create_mapping_late(start, (unsigned long)__init_begin,
> +                       end - start,
> +                       prot_sect_kernel | PMD_SECT_PXN, pgprot_default | PTE_PXN);
> +       if (!IS_ALIGNED(start, SECTION_SIZE) || !IS_ALIGNED(end, SECTION_SIZE))
> +               stop_machine(__flush_mappings, NULL, NULL);
> +}
> +
>  /*
>   * paging_init() sets up the page tables, initialises the zone memory
>   * maps and sets up the zero page.
> @@ -317,6 +457,7 @@ void __init paging_init(void)
>         void *zero_page;
>
>         map_mem();
> +       fixup_executable();
>
>         /*
>          * Finally flush the caches and tlb to ensure that we're in a
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
>

Cheers,
--
Steve