arm64: Treat handle_arch_irq as a function pointer

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 read only. Declare handle_arch_irq as a proper function pointer stored in the data section.

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
v4: style comments

---
 arch/arm64/include/asm/irq.h | 1 -
 arch/arm64/kernel/entry.S    | 6 ++----
 arch/arm64/kernel/irq.c      | 2 ++
 3 files changed, 4 insertions(+), 5 deletions(-)

arch/arm64/include/asm/irq.h +0 -1

index e1f7ecd..1eebf5b 100644
@@ -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 *));
 

arch/arm64/kernel/entry.S +2 -4

index 726b910..ee27f39 100644
@@ -168,7 +168,8 @@ tsk .req    x28             // current thread_info
  * Interrupt handling.
  */
        .macro  irq_handler
-       ldr     x1, handle_arch_irq
+       adrp    x1, handle_arch_irq
+       ldr     x1, [x1, #:lo12:handle_arch_irq]
        mov     x0, sp
        blr     x1
        .endm

@@ -695,6 +696,3 @@ ENTRY(sys_rt_sigreturn_wrapper)
        mov     x0, sp
        b       sys_rt_sigreturn
 ENDPROC(sys_rt_sigreturn_wrapper)
-
-ENTRY(handle_arch_irq)
-       .quad   0

arch/arm64/kernel/irq.c +2 -1

index 071a6ec..240b75c 100644
@@ -40,6 +40,8 @@ int arch_show_interrupts(struct seq_file *p, int prec)
        return 0;
 }
 
+void (*handle_arch_irq)(struct pt_regs *) = NULL;
+
 void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
 {
        if (handle_arch_irq)
--

Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 read only. Declare handle_arch_irq as a proper function pointer stored in the data section.

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
v4: style comments

---
 arch/arm64/include/asm/irq.h | 1 -
 arch/arm64/kernel/entry.S    | 6 ++----
 arch/arm64/kernel/irq.c      | 2 ++
 3 files changed, 4 insertions(+), 5 deletions(-)

arch/arm64/include/asm/irq.h +0 -1

index e1f7ecd..1eebf5b 100644
@@ -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 *));
 

arch/arm64/kernel/entry.S +2 -4

index 726b910..ee27f39 100644
@@ -168,7 +168,8 @@ tsk	.req	x28		// current thread_info
  * Interrupt handling.
  */
 	.macro	irq_handler
-	ldr	x1, handle_arch_irq
+	adrp	x1, handle_arch_irq
+	ldr	x1, [x1, #:lo12:handle_arch_irq]
 	mov	x0, sp
 	blr	x1
 	.endm

@@ -695,6 +696,3 @@ ENTRY(sys_rt_sigreturn_wrapper)
 	mov	x0, sp
 	b	sys_rt_sigreturn
 ENDPROC(sys_rt_sigreturn_wrapper)
-
-ENTRY(handle_arch_irq)
-	.quad	0

arch/arm64/kernel/irq.c +2 -0

index 071a6ec..240b75c 100644
@@ -40,6 +40,8 @@ int arch_show_interrupts(struct seq_file *p, int prec)
 	return 0;
 }
 
+void (*handle_arch_irq)(struct pt_regs *) = NULL;
+
 void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
 {
 	if (handle_arch_irq)

Signature · 2 lines

Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

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 adrp for getting the address to ensure we aren't affected by the location of the __hyp_stub_vectors.

Signed-off-by Laura Abbott <[email protected]>

---
v4: Commit text tweaks

---
 arch/arm64/kernel/head.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm64/kernel/head.S +2 -2

index 0a6e4f9..10f5cc0 100644
@@ -331,7 +331,8 @@ 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
+       adrp    x0, __hyp_stub_vectors
+       add     x0, x0, #:lo12:__hyp_stub_vectors
        msr     vbar_el2, x0
 
        /* spsr */
--

Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

adrp for getting the address to ensure we aren't affected by the location of the __hyp_stub_vectors.

Signed-off-by Laura Abbott <[email protected]>

---
v4: Commit text tweaks

---
 arch/arm64/kernel/head.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm64/kernel/head.S +2 -1

index 0a6e4f9..10f5cc0 100644
@@ -331,7 +331,8 @@ 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
+	adrp	x0, __hyp_stub_vectors
+	add	x0, x0, #:lo12:__hyp_stub_vectors
 	msr	vbar_el2, x0
 
 	/* spsr */

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 adrp for getting the address to ensure we aren't affected by the location of the __hyp_stub_vectors.

Signed-off-by Laura Abbott <[email protected]>

---
v4: Commit text tweaks

---
 arch/arm64/kernel/head.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm64/kernel/head.S +2 -1

index 0a6e4f9..10f5cc0 100644
@@ -331,7 +331,8 @@ 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
+	adrp	x0, __hyp_stub_vectors
+	add	x0, x0, #:lo12:__hyp_stub_vectors
 	msr	vbar_el2, x0
 
 	/* spsr */

Signature · 2 lines

Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

The function cpu_resume currently lives in the .data section. There's no reason for it to be there since we can use relative instructions without a problem. Move it to the text section where it belongs.

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
 arch/arm64/kernel/sleep.S | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

arch/arm64/kernel/sleep.S +0 -0

index a564b44..5762b16 100644
@@ -147,12 +147,12 @@ cpu_resume_after_mmu:
        ret

arch/arm64/kernel/sleep.S +2 -2

        ldr     x0, [x0, x7, lsl #3]
        /* load sp from context */
        ldr     x2, [x0, #CPU_CTX_SP]
-       adr     x1, sleep_idmap_phys
+       adrp    x1, sleep_idmap_phys
        /* load physical address of identity map page table in x1 */
-       ldr     x1, [x1]
+       ldr     x1, [x1, #:lo12:sleep_idmap_phys]
        mov     sp, x2
        /*
         * cpu_do_resume expects x0 to contain context physical address

Quoted 11 lines · Laura Abbott

The function cpu_resume currently lives in the .data section. There's no reason for it to be there since we can use relative instructions without a problem. Move it to the text section where it belongs.

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
 arch/arm64/kernel/sleep.S | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Hi Laura,

Apologies for waiting until v4 to bring this up, but I have some minor comments.

Quoted 8 lines

arch/arm64/kernel/sleep.S +0 -0

index a564b44..5762b16 100644
@@ -147,12 +147,12 @@ cpu_resume_after_mmu:
        ret
 ENDPROC(cpu_resume_after_mmu)
 

arch/arm64/kernel/sleep.S cpu_resume_after_mmu: · line 155

Quoted 8 lines

arch/arm64/kernel/sleep.S +2 -2

-       .data
 ENTRY(cpu_resume)
        bl      el2_setup               // if in EL2 drop to EL1 cleanly
 #ifdef CONFIG_SMP
        mrs     x1, mpidr_el1
-       adr     x4, mpidr_hash_ptr
+       adrp    x4, mpidr_hash_ptr
+       add     x4, x4, #:lo12:mpidr_hash_ptr

Instead of this change, you could put mpidr_hash_ptr itself in .text as well.

arch/arm64/kernel/sleep.S ENTRY(cpu_resume) · line 169

Quoted 11 lines

arch/arm64/kernel/sleep.S +2 -1

        ldr     x5, [x4]
        add     x8, x4, x5              // x8 = struct mpidr_hash phys address
         /* retrieve mpidr_hash members to compute the hash */

@@ -164,14 +164,15 @@ ENTRY(cpu_resume)
 #else
        mov     x7, xzr
 #endif
-       adr     x0, sleep_save_sp
+       adrp    x0, sleep_save_sp
+       add     x0, x0, #:lo12:sleep_save_sp
        ldr     x0, [x0, #SLEEP_SAVE_SP_PHYS]

Nit:
ldr x0, [x0, #:lo12:sleep_save_sp + SLEEP_SAVE_SP_PHYS]

Quoted 12 lines

arch/arm64/kernel/sleep.S +2 -2

        ldr     x0, [x0, x7, lsl #3]
        /* load sp from context */
        ldr     x2, [x0, #CPU_CTX_SP]
-       adr     x1, sleep_idmap_phys
+       adrp    x1, sleep_idmap_phys
        /* load physical address of identity map page table in x1 */
-       ldr     x1, [x1]
+       ldr     x1, [x1, #:lo12:sleep_idmap_phys]
        mov     sp, x2
        /*
         * cpu_do_resume expects x0 to contain context physical address

arch/arm64/kernel/sleep.S ENTRY(cpu_resume) · line 188

Quoted 7 lines

arch/arm64/kernel/sleep.S +1 -0

@@ -181,6 +182,7 @@ ENTRY(cpu_resume)
        b       cpu_resume_mmu          // Resume MMU, never returns
 ENDPROC(cpu_resume)
 
+       .data
        .align 3
 mpidr_hash_ptr:
        /*

Acked-by Ard Biesheuvel <[email protected]>

Quoted 25 lines · Ard Biesheuvel

Quoted 11 lines · Laura Abbott

The function cpu_resume currently lives in the .data section. There's no reason for it to be there since we can use relative instructions without a problem. Move it to the text section where it belongs.

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
 arch/arm64/kernel/sleep.S | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Hi Laura,

Apologies for waiting until v4 to bring this up, but I have some minor comments.

Quoted 8 lines

arch/arm64/kernel/sleep.S +0 -0

index a564b44..5762b16 100644
@@ -147,12 +147,12 @@ cpu_resume_after_mmu:
        ret
 ENDPROC(cpu_resume_after_mmu)
 

Quoted 11 lines

arch/arm64/kernel/sleep.S cpu_resume_after_mmu: · line 155

Quoted 8 lines

arch/arm64/kernel/sleep.S +2 -2

-       .data
 ENTRY(cpu_resume)
        bl      el2_setup               // if in EL2 drop to EL1 cleanly
 #ifdef CONFIG_SMP
        mrs     x1, mpidr_el1
-       adr     x4, mpidr_hash_ptr
+       adrp    x4, mpidr_hash_ptr
+       add     x4, x4, #:lo12:mpidr_hash_ptr

Instead of this change, you could put mpidr_hash_ptr itself in .text as well.

Actually, looking more closely, it appears mpidr_hash_ptr can be dropped completely:
the address obtained by adrp/add is PC relative, so the whole sequence could just be

adrp x8, mpidr_hash
add x8, x8, #:lo12:mpidr_hash // x8 = struct mpidr_hash phys address

(and the ldr and add below can be dropped, and so can mpidr_hash_ptr at the end)

Quoted 43 lines · Ard Biesheuvel

Quoted 25 lines · Ard Biesheuvel

Quoted 11 lines · Laura Abbott

The function cpu_resume currently lives in the .data section. There's no reason for it to be there since we can use relative instructions without a problem. Move it to the text section where it belongs.

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
 arch/arm64/kernel/sleep.S | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Hi Laura,

Apologies for waiting until v4 to bring this up, but I have some minor comments.

Quoted 8 lines

arch/arm64/kernel/sleep.S +0 -0

index a564b44..5762b16 100644
@@ -147,12 +147,12 @@ cpu_resume_after_mmu:
        ret
 ENDPROC(cpu_resume_after_mmu)
 

Quoted 11 lines

arch/arm64/kernel/sleep.S cpu_resume_after_mmu: · line 155

Quoted 8 lines

arch/arm64/kernel/sleep.S +2 -2

-       .data
 ENTRY(cpu_resume)
        bl      el2_setup               // if in EL2 drop to EL1 cleanly
 #ifdef CONFIG_SMP
        mrs     x1, mpidr_el1
-       adr     x4, mpidr_hash_ptr
+       adrp    x4, mpidr_hash_ptr
+       add     x4, x4, #:lo12:mpidr_hash_ptr

Instead of this change, you could put mpidr_hash_ptr itself in .text as well.

Actually, looking more closely, it appears mpidr_hash_ptr can be dropped completely:
the address obtained by adrp/add is PC relative, so the whole sequence could just be

Quoted 4 lines

adrp x8, mpidr_hash
add x8, x8, #:lo12:mpidr_hash // x8 = struct mpidr_hash phys address

(and the ldr and add below can be dropped, and so can mpidr_hash_ptr at the end)

Also, if you update arch/arm64/kernel/suspend.c to drop the extern from sleep_save_sp and sleep_idmap_phys, you can drop all of the .data from sleep.S

I gave all of that a go locally and my Juno happily booted to userspace with cpuidle running. I had to apply Marc's timer fix to get cpuidle to work at all with v3.18-rc2, but that should be fixed for -rc3.

So with those changes:

Reviewed-by Mark Rutland <[email protected]>

Tested-by Mark Rutland <[email protected]>

Quoted 49 lines · Mark Rutland

Quoted 47 lines · Ard Biesheuvel

Quoted 25 lines · Ard Biesheuvel

Quoted 11 lines · Laura Abbott

The function cpu_resume currently lives in the .data section. There's no reason for it to be there since we can use relative instructions without a problem. Move it to the text section where it belongs.

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
 arch/arm64/kernel/sleep.S | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Hi Laura,

Apologies for waiting until v4 to bring this up, but I have some minor comments.

Quoted 8 lines

arch/arm64/kernel/sleep.S +0 -0

index a564b44..5762b16 100644
@@ -147,12 +147,12 @@ cpu_resume_after_mmu:
        ret
 ENDPROC(cpu_resume_after_mmu)
 

Quoted 11 lines

arch/arm64/kernel/sleep.S cpu_resume_after_mmu: · line 155

Quoted 8 lines

arch/arm64/kernel/sleep.S +2 -2

-       .data
 ENTRY(cpu_resume)
        bl      el2_setup               // if in EL2 drop to EL1 cleanly
 #ifdef CONFIG_SMP
        mrs     x1, mpidr_el1
-       adr     x4, mpidr_hash_ptr
+       adrp    x4, mpidr_hash_ptr
+       add     x4, x4, #:lo12:mpidr_hash_ptr

Instead of this change, you could put mpidr_hash_ptr itself in .text as well.

Actually, looking more closely, it appears mpidr_hash_ptr can be dropped completely:
the address obtained by adrp/add is PC relative, so the whole sequence could just be

adrp x8, mpidr_hash
add x8, x8, #:lo12:mpidr_hash // x8 = struct mpidr_hash phys address

(and the ldr and add below can be dropped, and so can mpidr_hash_ptr at the end)

Quoted 3 lines

Also, if you update arch/arm64/kernel/suspend.c to drop the extern from sleep_save_sp and sleep_idmap_phys, you can drop all of the .data from sleep.S

If with that you also mean that all adr references to those data structures in the resume path should become adrp I agree otherwise we might have an offset issue.

Quoted 54 lines · Lorenzo Pieralisi

Quoted 52 lines · Mark Rutland

Quoted 43 lines · Ard Biesheuvel

Quoted 25 lines · Ard Biesheuvel

Quoted 11 lines · Laura Abbott

The function cpu_resume currently lives in the .data section. There's no reason for it to be there since we can use relative instructions without a problem. Move it to the text section where it belongs.

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
  arch/arm64/kernel/sleep.S | 12 +++++++-----
  1 file changed, 7 insertions(+), 5 deletions(-)

Hi Laura,

Apologies for waiting until v4 to bring this up, but I have some minor comments.

Quoted 8 lines

arch/arm64/kernel/sleep.S +0 -0

index a564b44..5762b16 100644
@@ -147,12 +147,12 @@ cpu_resume_after_mmu:
         ret
  ENDPROC(cpu_resume_after_mmu)
 

Quoted 11 lines

arch/arm64/kernel/sleep.S cpu_resume_after_mmu: · line 155

Quoted 8 lines

arch/arm64/kernel/sleep.S +2 -2

-       .data
  ENTRY(cpu_resume)
         bl      el2_setup               // if in EL2 drop to EL1 cleanly
  #ifdef CONFIG_SMP
         mrs     x1, mpidr_el1
-       adr     x4, mpidr_hash_ptr
+       adrp    x4, mpidr_hash_ptr
+       add     x4, x4, #:lo12:mpidr_hash_ptr

Instead of this change, you could put mpidr_hash_ptr itself in .text as well.

Actually, looking more closely, it appears mpidr_hash_ptr can be dropped completely:
the address obtained by adrp/add is PC relative, so the whole sequence could just be

Quoted 4 lines

adrp x8, mpidr_hash
add x8, x8, #:lo12:mpidr_hash // x8 = struct mpidr_hash phys address

(and the ldr and add below can be dropped, and so can mpidr_hash_ptr at the end)

Also, if you update arch/arm64/kernel/suspend.c to drop the extern from sleep_save_sp and sleep_idmap_phys, you can drop all of the .data from sleep.S

Quoted 3 lines

If with that you also mean that all adr references to those data structures in the resume path should become adrp I agree otherwise we might have an offset issue.

Yes. The original patch had those modifications already, so I assumed they would remain.

The head.text section is intended to be run at early bootup before any of the regular kernel mappings have been setup. Parts of head.text may be freed back into the buddy allocator due to TEXT_OFFSET so for security requirements this memory must not be executable. The suspend/resume/hotplug code path requires some of these head.S functions to run however which means they need to be executable. Support these conflicting requirements by moving the few head.text functions that need to be executable to the text section which has the appropriate page table permissions.

Signed-off-by Laura Abbott <[email protected]>

---
v4: New apprach based on discussions with Mark

---
 arch/arm64/kernel/head.S | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm64/kernel/head.S +2 -0

index 10f5cc0..dc362da 100644
@@ -432,12 +432,14 @@ ENTRY(secondary_startup)
        b       __enable_mmu
 ENDPROC(secondary_startup)
 
+       .pushsection    .text, "ax"
 ENTRY(__secondary_switched)
        ldr     x0, [x21]                       // get secondary_data.stack
        mov     sp, x0
        mov     x29, #0
        b       secondary_start_kernel
 ENDPROC(__secondary_switched)
+       .popsection
 #endif /* CONFIG_SMP */
 
 /*

arch/arm64/kernel/head.S +1 -1

 __turn_mmu_on:
        msr     sctlr_el1, x0
        isb
        br      x27
 ENDPROC(__turn_mmu_on)
+       .popsection
 
 /*
  * Calculate the start of physical memory.
--

Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 36 lines · Laura Abbott

The head.text section is intended to be run at early bootup before any of the regular kernel mappings have been setup. Parts of head.text may be freed back into the buddy allocator due to TEXT_OFFSET so for security requirements this memory must not be executable. The suspend/resume/hotplug code path requires some of these head.S functions to run however which means they need to be executable. Support these conflicting requirements by moving the few head.text functions that need to be executable to the text section which has the appropriate page table permissions.

Signed-off-by Laura Abbott <[email protected]>

---
v4: New apprach based on discussions with Mark

---
 arch/arm64/kernel/head.S | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm64/kernel/head.S +2 -0

index 10f5cc0..dc362da 100644
@@ -432,12 +432,14 @@ ENTRY(secondary_startup)
        b       __enable_mmu
 ENDPROC(secondary_startup)
 
+       .pushsection    .text, "ax"
 ENTRY(__secondary_switched)
        ldr     x0, [x21]                       // get secondary_data.stack
        mov     sp, x0
        mov     x29, #0
        b       secondary_start_kernel
 ENDPROC(__secondary_switched)
+       .popsection
 #endif /* CONFIG_SMP */
 

arch/arm64/kernel/head.S ENDPROC(__enable_mmu) · line 476

Quoted 6 lines

arch/arm64/kernel/head.S +1 -0

 /*

@@ -471,11 +473,13 @@ ENDPROC(__enable_mmu)
  * table to map the entire function.
  */
        .align  4
+       .pushsection    .text, "ax"

There is a comment before this .align that explains why it is separated from __enable_mmu, and I think jumping into another section right after it kind of defeats the purpose.
Perhaps it is better to put the pushsection before __enable_mmu instead?

Quoted 19 lines

arch/arm64/kernel/head.S +1 -1

 __turn_mmu_on:
        msr     sctlr_el1, x0
        isb
        br      x27
 ENDPROC(__turn_mmu_on)
+       .popsection
 
 /*
  * Calculate the start of physical memory.
--

Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 37 lines · Ard Biesheuvel

Quoted 36 lines · Laura Abbott

The head.text section is intended to be run at early bootup before any of the regular kernel mappings have been setup. Parts of head.text may be freed back into the buddy allocator due to TEXT_OFFSET so for security requirements this memory must not be executable. The suspend/resume/hotplug code path requires some of these head.S functions to run however which means they need to be executable. Support these conflicting requirements by moving the few head.text functions that need to be executable to the text section which has the appropriate page table permissions.

Signed-off-by Laura Abbott <[email protected]>

---
v4: New apprach based on discussions with Mark

---
  arch/arm64/kernel/head.S | 4 ++++
  1 file changed, 4 insertions(+)

arch/arm64/kernel/head.S +2 -0

index 10f5cc0..dc362da 100644
@@ -432,12 +432,14 @@ ENTRY(secondary_startup)
         b       __enable_mmu
  ENDPROC(secondary_startup)
 
+       .pushsection    .text, "ax"
  ENTRY(__secondary_switched)
         ldr     x0, [x21]                       // get secondary_data.stack
         mov     sp, x0
         mov     x29, #0
         b       secondary_start_kernel
  ENDPROC(__secondary_switched)
+       .popsection
  #endif /* CONFIG_SMP */
 

Quoted 11 lines

arch/arm64/kernel/head.S ENDPROC(__enable_mmu) · line 476

Quoted 6 lines

arch/arm64/kernel/head.S +1 -0

  /*

@@ -471,11 +473,13 @@ ENDPROC(__enable_mmu)
   * table to map the entire function.
   */
         .align  4
+       .pushsection    .text, "ax"

There is a comment before this .align that explains why it is separated from __enable_mmu, and I think jumping into another section right after it kind of defeats the purpose.
Perhaps it is better to put the pushsection before __enable_mmu instead?

To keep the alignment correct we just need to move the .align after the pushsection. With that changed I think this patch is Ok.

As __enable_mmu is only executed with the MMU off it doesn't need to be moved into an executable section to prevent the MMU from blowing up in our faces -- it would be wrong to call it with the MMU on anyway.

However, this does raise a potential problem in that an attacker could scribble over code executed before the MMU is on. Then they just have to wait for the next CPU hotplug or suspend/resume for it to be executed. So some functions including __enable_mmu and el2_setup aren't necessarily safe in their current location.

There are a few ways of solving that, either moving stuff around or releasing less memory for allocation.

Quoted 57 lines · Mark Rutland

Quoted 37 lines · Ard Biesheuvel

Quoted 36 lines · Laura Abbott

The head.text section is intended to be run at early bootup before any of the regular kernel mappings have been setup. Parts of head.text may be freed back into the buddy allocator due to TEXT_OFFSET so for security requirements this memory must not be executable. The suspend/resume/hotplug code path requires some of these head.S functions to run however which means they need to be executable. Support these conflicting requirements by moving the few head.text functions that need to be executable to the text section which has the appropriate page table permissions.

Signed-off-by Laura Abbott <[email protected]>

---
v4: New apprach based on discussions with Mark

---
  arch/arm64/kernel/head.S | 4 ++++
  1 file changed, 4 insertions(+)

arch/arm64/kernel/head.S +2 -0

index 10f5cc0..dc362da 100644
@@ -432,12 +432,14 @@ ENTRY(secondary_startup)
         b       __enable_mmu
  ENDPROC(secondary_startup)
 
+       .pushsection    .text, "ax"
  ENTRY(__secondary_switched)
         ldr     x0, [x21]                       // get secondary_data.stack
         mov     sp, x0
         mov     x29, #0
         b       secondary_start_kernel
  ENDPROC(__secondary_switched)
+       .popsection
  #endif /* CONFIG_SMP */
 

Quoted 11 lines

arch/arm64/kernel/head.S ENDPROC(__enable_mmu) · line 476

Quoted 6 lines

arch/arm64/kernel/head.S +1 -0

  /*

@@ -471,11 +473,13 @@ ENDPROC(__enable_mmu)
   * table to map the entire function.
   */
         .align  4
+       .pushsection    .text, "ax"

There is a comment before this .align that explains why it is separated from __enable_mmu, and I think jumping into another section right after it kind of defeats the purpose.
Perhaps it is better to put the pushsection before __enable_mmu instead?

To keep the alignment correct we just need to move the .align after the pushsection. With that changed I think this patch is Ok.

As __enable_mmu is only executed with the MMU off it doesn't need to be moved into an executable section to prevent the MMU from blowing up in our faces -- it would be wrong to call it with the MMU on anyway.

Quoted 9 lines

However, this does raise a potential problem in that an attacker could scribble over code executed before the MMU is on. Then they just have to wait for the next CPU hotplug or suspend/resume for it to be executed. So some functions including __enable_mmu and el2_setup aren't necessarily safe in their current location.

There are a few ways of solving that, either moving stuff around or releasing less memory for allocation.

My original version of the patch did move everything around to keep __enable_mmu and others into the text section so they couldn't be modified. It seemed rather unwieldy though so I went with this approach. Releasing less memory for allocation is less than optimal so I'd prefer if we moved code around to the appropriate section. Any opinions about going back to my original approach of moving things around vs. going with more pushsection annotations?

The fixmap API was originally added for arm64 for early_ioremap purposes. It can be used for other purposes too so move the initialization from ioremap to somewhere more generic. This makes it obvious where the fixmap is being set up and allows for a cleaner implementation of __set_fixmap.

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
 arch/arm64/include/asm/fixmap.h |  7 +--
 arch/arm64/kernel/setup.c       |  3 +-
 arch/arm64/mm/ioremap.c         | 93 ++--------------------------------------
 arch/arm64/mm/mmu.c             | 94 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 103 insertions(+), 94 deletions(-)

arch/arm64/include/asm/fixmap.h +4 -3

index 5f7bfe6..db26a2f2 100644
@@ -56,10 +56,11 @@ enum fixed_addresses {
 
 #define FIXMAP_PAGE_IO     __pgprot(PROT_DEVICE_nGnRE)
 
-extern void __early_set_fixmap(enum fixed_addresses idx,
-			       phys_addr_t phys, pgprot_t flags);
+void __init early_fixmap_init(void);
 
-#define __set_fixmap __early_set_fixmap
+#define __early_set_fixmap __set_fixmap
+
+extern void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot);
 
 #include <asm-generic/fixmap.h>
 

arch/arm64/kernel/setup.c +2 -1

index 2437196..efc8bc5 100644
@@ -376,7 +376,8 @@ void __init setup_arch(char **cmdline_p)
 
 	*cmdline_p = boot_command_line;
 
-	early_ioremap_init();
+	early_fixmap_init();
+	early_ioremap_setup();
 
 	parse_early_param();
 

arch/arm64/mm/ioremap.c +3 -90

index fa324bd..cbb99c8 100644
@@ -103,97 +103,10 @@ void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size)
 }
 EXPORT_SYMBOL(ioremap_cache);
 
-static pte_t bm_pte[PTRS_PER_PTE] __page_aligned_bss;
-#if CONFIG_ARM64_PGTABLE_LEVELS > 2
-static pte_t bm_pmd[PTRS_PER_PMD] __page_aligned_bss;
-#endif
-#if CONFIG_ARM64_PGTABLE_LEVELS > 3
-static pte_t bm_pud[PTRS_PER_PUD] __page_aligned_bss;
-#endif
-
-static inline pud_t * __init early_ioremap_pud(unsigned long addr)
-{
-	pgd_t *pgd;
-
-	pgd = pgd_offset_k(addr);
-	BUG_ON(pgd_none(*pgd) || pgd_bad(*pgd));
-
-	return pud_offset(pgd, addr);
-}
-
-static inline pmd_t * __init early_ioremap_pmd(unsigned long addr)
-{
-	pud_t *pud = early_ioremap_pud(addr);
-
-	BUG_ON(pud_none(*pud) || pud_bad(*pud));
-
-	return pmd_offset(pud, addr);
-}
-
-static inline pte_t * __init early_ioremap_pte(unsigned long addr)
-{
-	pmd_t *pmd = early_ioremap_pmd(addr);
-
-	BUG_ON(pmd_none(*pmd) || pmd_bad(*pmd));
-
-	return pte_offset_kernel(pmd, addr);
-}
-
+/*
+ * Must be called after early_fixmap_init
+ */
 void __init early_ioremap_init(void)
 {
-	pgd_t *pgd;
-	pud_t *pud;
-	pmd_t *pmd;
-	unsigned long addr = fix_to_virt(FIX_BTMAP_BEGIN);
-
-	pgd = pgd_offset_k(addr);
-	pgd_populate(&init_mm, pgd, bm_pud);
-	pud = pud_offset(pgd, addr);
-	pud_populate(&init_mm, pud, bm_pmd);
-	pmd = pmd_offset(pud, addr);
-	pmd_populate_kernel(&init_mm, pmd, bm_pte);
-
-	/*
-	 * The boot-ioremap range spans multiple pmds, for which
-	 * we are not prepared:
-	 */
-	BUILD_BUG_ON((__fix_to_virt(FIX_BTMAP_BEGIN) >> PMD_SHIFT)
-		     != (__fix_to_virt(FIX_BTMAP_END) >> PMD_SHIFT));
-
-	if (pmd != early_ioremap_pmd(fix_to_virt(FIX_BTMAP_END))) {
-		WARN_ON(1);
-		pr_warn("pmd %p != %p\n",
-			pmd, early_ioremap_pmd(fix_to_virt(FIX_BTMAP_END)));
-		pr_warn("fix_to_virt(FIX_BTMAP_BEGIN): %08lx\n",
-			fix_to_virt(FIX_BTMAP_BEGIN));
-		pr_warn("fix_to_virt(FIX_BTMAP_END):   %08lx\n",
-			fix_to_virt(FIX_BTMAP_END));
-
-		pr_warn("FIX_BTMAP_END:       %d\n", FIX_BTMAP_END);
-		pr_warn("FIX_BTMAP_BEGIN:     %d\n",
-			FIX_BTMAP_BEGIN);
-	}
-
 	early_ioremap_setup();
 }
-
-void __init __early_set_fixmap(enum fixed_addresses idx,
-			       phys_addr_t phys, pgprot_t flags)
-{
-	unsigned long addr = __fix_to_virt(idx);
-	pte_t *pte;
-
-	if (idx >= __end_of_fixed_addresses) {
-		BUG();
-		return;
-	}
-
-	pte = early_ioremap_pte(addr);
-
-	if (pgprot_val(flags))
-		set_pte(pte, pfn_pte(phys >> PAGE_SHIFT, flags));
-	else {
-		pte_clear(&init_mm, addr, pte);
-		flush_tlb_kernel_range(addr, addr+PAGE_SIZE);
-	}
-}

arch/arm64/mm/mmu.c +94 -0

index 6894ef3..e92f633 100644
@@ -28,6 +28,7 @@
 #include <linux/io.h>
 
 #include <asm/cputype.h>
+#include <asm/fixmap.h>
 #include <asm/sections.h>
 #include <asm/setup.h>
 #include <asm/sizes.h>

@@ -459,3 +460,96 @@ void vmemmap_free(unsigned long start, unsigned long end)
 {
 }
 #endif	/* CONFIG_SPARSEMEM_VMEMMAP */
+
+static pte_t bm_pte[PTRS_PER_PTE] __page_aligned_bss;
+#if CONFIG_ARM64_PGTABLE_LEVELS > 2
+static pte_t bm_pmd[PTRS_PER_PMD] __page_aligned_bss;
+#endif
+#if CONFIG_ARM64_PGTABLE_LEVELS > 3
+static pte_t bm_pud[PTRS_PER_PUD] __page_aligned_bss;
+#endif
+
+static inline pud_t * fixmap_pud(unsigned long addr)
+{
+	pgd_t *pgd = pgd_offset_k(addr);
+
+	BUG_ON(pgd_none(*pgd) || pgd_bad(*pgd));
+
+	return pud_offset(pgd, addr);
+}
+
+static inline pmd_t * fixmap_pmd(unsigned long addr)
+{
+	pud_t *pud = fixmap_pud(addr);
+
+	BUG_ON(pud_none(*pud) || pud_bad(*pud));
+
+	return pmd_offset(pud, addr);
+}
+
+static inline pte_t * fixmap_pte(unsigned long addr)
+{
+	pmd_t *pmd = fixmap_pmd(addr);
+
+	BUG_ON(pmd_none(*pmd) || pmd_bad(*pmd));
+
+	return pte_offset_kernel(pmd, addr);
+}
+
+void __init early_fixmap_init(void)
+{
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+	unsigned long addr = FIXADDR_START;
+
+	pgd = pgd_offset_k(addr);
+	pgd_populate(&init_mm, pgd, bm_pud);
+	pud = pud_offset(pgd, addr);
+	pud_populate(&init_mm, pud, bm_pmd);
+	pmd = pmd_offset(pud, addr);
+	pmd_populate_kernel(&init_mm, pmd, bm_pte);
+
+	/*
+	 * The boot-ioremap range spans multiple pmds, for which
+	 * we are not preparted:
+	 */
+	BUILD_BUG_ON((__fix_to_virt(FIX_BTMAP_BEGIN) >> PMD_SHIFT)
+		     != (__fix_to_virt(FIX_BTMAP_END) >> PMD_SHIFT));
+
+	if ((pmd != fixmap_pmd(fix_to_virt(FIX_BTMAP_BEGIN)))
+	     || pmd != fixmap_pmd(fix_to_virt(FIX_BTMAP_END))) {
+		WARN_ON(1);
+		pr_warn("pmd %p != %p, %p\n",
+			pmd, fixmap_pmd(fix_to_virt(FIX_BTMAP_BEGIN)),
+			fixmap_pmd(fix_to_virt(FIX_BTMAP_END)));
+		pr_warn("fix_to_virt(FIX_BTMAP_BEGIN): %08lx\n",
+			fix_to_virt(FIX_BTMAP_BEGIN));
+		pr_warn("fix_to_virt(FIX_BTMAP_END):   %08lx\n",
+			fix_to_virt(FIX_BTMAP_END));
+
+		pr_warn("FIX_BTMAP_END:       %d\n", FIX_BTMAP_END);
+		pr_warn("FIX_BTMAP_BEGIN:     %d\n", FIX_BTMAP_BEGIN);
+	}
+}
+
+void __set_fixmap(enum fixed_addresses idx,
+			       phys_addr_t phys, pgprot_t flags)
+{
+	unsigned long addr = __fix_to_virt(idx);
+	pte_t *pte;
+
+	if (idx >= __end_of_fixed_addresses) {
+		BUG();
+		return;
+	}
+
+	pte = fixmap_pte(addr);
+
+	if (pgprot_val(flags)) {
+		set_pte(pte, pfn_pte(phys >> PAGE_SHIFT, flags));
+	} else {
+		pte_clear(&init_mm, addr, pte);
+		flush_tlb_kernel_range(addr, addr+PAGE_SIZE);
+	}
+}

Signature · 2 lines

Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Add page protections for arm64 similar to those in arm. This is for security reasons to prevent certain classes of exploits. The current method:

- 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 <[email protected]>

---
v4: Combined the Kconfig options

---
 arch/arm64/Kconfig.debug            |  23 +++
 arch/arm64/include/asm/cacheflush.h |   4 +
 arch/arm64/kernel/vmlinux.lds.S     |  17 ++
 arch/arm64/mm/init.c                |   1 +
 arch/arm64/mm/mm.h                  |   2 +
 arch/arm64/mm/mmu.c                 | 303 +++++++++++++++++++++++++++++++-----
 6 files changed, 314 insertions(+), 36 deletions(-)

arch/arm64/Kconfig.debug +23 -0

index 0a12933..1b25015 100644
@@ -54,4 +54,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

arch/arm64/include/asm/cacheflush.h +4 -0

index 689b637..0014207 100644
@@ -152,4 +152,8 @@ 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);
+
+#ifdef CONFIG_DEBUG_RODATA
+void mark_rodata_ro(void);
+#endif
 #endif

arch/arm64/kernel/vmlinux.lds.S +17 -0

index edf8715..83424ad 100644
@@ -8,6 +8,7 @@
 #include <asm/thread_info.h>
 #include <asm/memory.h>
 #include <asm/page.h>
+#include <asm/pgtable.h>
 
 #include "image.h"
 

@@ -54,6 +55,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 = .;

@@ -70,19 +74,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)

arch/arm64/mm/init.c +1 -0

index 494297c..61f44c7 100644
@@ -324,6 +324,7 @@ void __init mem_init(void)
 
 void free_initmem(void)
 {
+	fixup_init();
 	free_initmem_default(0);
 }
 

arch/arm64/mm/mm.h +2 -0

index d519f4f..82347d8 100644
@@ -1,2 +1,4 @@
 extern void __init bootmem_init(void);
 extern void __init arm64_swiotlb_init(void);
+
+void fixup_init(void);

arch/arm64/mm/mmu.c +43 -4

index e92f633..0428294 100644
@@ -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/fixmap.h>

@@ -137,17 +138,55 @@ static void __init *early_alloc(unsigned long sz)
 	return ptr;
 }
 
-static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
+/*
+ * remap a PMD into pages
+ */
+static noinline void __ref split_pmd(pmd_t *pmd, bool early)
+{
+	pte_t *pte, *start_pte;
+	unsigned long pfn;
+	int i = 0;
+
+	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 = pmd_pfn(*pmd);
+
+	do {
+		/*
+		 * Need to have the least restrictive permissions available
+		 * permissions will be fixed up later
+		 */
+		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)
+				  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, early);
 
 	pte = pte_offset_kernel(pmd, addr);
 	do {

@@ -156,29 +195,52 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
 	} while (pte++, addr += PAGE_SIZE, addr != end);

arch/arm64/mm/mmu.c +11 -3

+	int i = 0;
+
+	do {
+		set_pmd(pmd, pfn_pmd(pfn, prot));
+		pfn++;
+	} while (pmd++, i++, i < PTRS_PER_PMD);
+}
+
+static void __ref alloc_init_pmd(pud_t *pud, unsigned long addr,
 				  unsigned long end, phys_addr_t phys,
-				  int map_io)
+				  pgprot_t sect_prot, pgprot_t pte_prot,
+				  bool early, int map_io)
 {
 	pmd_t *pmd;
 	unsigned long next;
-	pmdval_t prot_sect;
-	pgprot_t prot_pte;

arch/arm64/mm/mmu.c +204 -23

 	/*
 	 * 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);
+		if (pud_sect(*pud)) {
+			/*
+			 * need to have the 1G of mappings continue to be
+			 * present
+			 */
+			split_pud(pud, pmd);
+		}
 		pud_populate(&init_mm, pud, pmd);
+		flush_tlb_all();
 	}
 
 	pmd = pmd_offset(pud, addr);

@@ -186,8 +248,8 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
 		next = pmd_addr_end(addr, end);
 		/* try section mapping first */
 		if (((addr | next | phys) & ~SECTION_MASK) == 0) {
-			pmd_t old_pmd =*pmd;
-			set_pmd(pmd, __pmd(phys | prot_sect));
+			pmd_t old_pmd = *pmd;
+			set_pmd(pmd, __pmd(phys | sect_prot));
 			/*
 			 * Check for previous table entries created during
 			 * boot (__create_page_tables) and flush them.

@@ -196,15 +258,16 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
 				flush_tlb_all();
 		} else {
 			alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys),
-				       prot_pte);
+					pte_prot, early);
 		}
 		phys += next - addr;
 	} while (pmd++, addr = next, addr != end);
 }
 
-static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,
+static void __ref alloc_init_pud(pgd_t *pgd, unsigned long addr,
 				  unsigned long end, unsigned long phys,
-				  int map_io)
+				  pgprot_t sect_prot, pgprot_t pte_prot,
+				  bool early, int map_io)
 {
 	pud_t *pud;
 	unsigned long next;

@@ -222,10 +285,10 @@ static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,
 		/*
 		 * For 4K granule only, attempt to put down a 1GB block
 		 */
-		if (!map_io && (PAGE_SHIFT == 12) &&
+		if (!map_io && early && (PAGE_SHIFT == 12) &&
 		    ((addr | next | phys) & ~PUD_MASK) == 0) {
 			pud_t old_pud = *pud;
-			set_pud(pud, __pud(phys | PROT_SECT_NORMAL_EXEC));
+			set_pud(pud, __pud(phys | sect_prot));
 
 			/*
 			 * If we have an old value for a pud, it will

@@ -240,7 +303,8 @@ static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,
 				flush_tlb_all();
 			}
 		} else {
-			alloc_init_pmd(pud, addr, next, phys, map_io);
+			alloc_init_pmd(pud, addr, next, phys, sect_prot,
+					pte_prot, early, map_io);
 		}
 		phys += next - addr;
 	} while (pud++, addr = next, addr != end);

@@ -250,9 +314,11 @@ 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(pgd_t *pgd, phys_addr_t phys,
-				    unsigned long virt, phys_addr_t size,
-				    int map_io)
+static void __ref __create_mapping(pgd_t *pgd, phys_addr_t phys,
+				  unsigned long virt,
+				  phys_addr_t size,
+				  pgprot_t sect_prot, pgprot_t pte_prot,
+				  int map_io, bool early)
 {
 	unsigned long addr, length, end, next;
 

@@ -262,31 +328,140 @@ static void __init __create_mapping(pgd_t *pgd, phys_addr_t phys,
 	end = addr + length;
 	do {
 		next = pgd_addr_end(addr, end);
-		alloc_init_pud(pgd, addr, next, phys, map_io);
+		alloc_init_pud(pgd, addr, next, phys, sect_prot, pte_prot,
+				early, map_io);
 		phys += next - addr;
 	} while (pgd++, addr = next, addr != end);
 }
 
-static void __init create_mapping(phys_addr_t phys, unsigned long virt,
-				  phys_addr_t size)
+void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io)
+{
+	if ((addr >> PGDIR_SHIFT) >= ARRAY_SIZE(idmap_pg_dir)) {
+		pr_warn("BUG: not creating id mapping for %pa\n", &addr);
+		return;
+	}
+	__create_mapping(&idmap_pg_dir[pgd_index(addr)],
+			 addr, addr, size, PROT_SECT_NORMAL_EXEC,
+			 PAGE_KERNEL_EXEC, map_io, false);
+}
+
+static inline pmd_t *pmd_off_k(unsigned long virt)
+{
+	return pmd_offset(pud_offset(pgd_offset_k(virt), virt), virt);
+}
+
+#ifdef CONFIG_EARLY_PRINTK
+/*
+ * Create an early I/O mapping using the pgd/pmd entries already populated
+ * in head.S as this function is called too early to allocated any memory. The
+ * mapping size is 2MB with 4KB pages or 64KB or 64KB pages.
+ */
+void __iomem * __init early_io_map(phys_addr_t phys, unsigned long virt)
+{
+	unsigned long size, mask;
+	bool page64k = IS_ENABLED(CONFIG_ARM64_64K_PAGES);
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+	pte_t *pte;
+
+	/*
+	 * No early pte entries with !ARM64_64K_PAGES configuration, so using
+	 * sections (pmd).
+	 */
+	size = page64k ? PAGE_SIZE : SECTION_SIZE;
+	mask = ~(size - 1);
+
+	pgd = pgd_offset_k(virt);
+	pud = pud_offset(pgd, virt);
+	if (pud_none(*pud))
+		return NULL;
+	pmd = pmd_offset(pud, virt);
+
+	if (page64k) {
+		if (pmd_none(*pmd))
+			return NULL;
+		pte = pte_offset_kernel(pmd, virt);
+		set_pte(pte, __pte((phys & mask) | PROT_DEVICE_nGnRE));
+	} else {
+		set_pmd(pmd, __pmd((phys & mask) | PROT_SECT_DEVICE_nGnRE));
+	}
+
+	return (void __iomem *)((virt & mask) + (phys & ~mask));
+}
+#endif
+
+static void __ref create_mapping(phys_addr_t phys, unsigned long virt,
+				  phys_addr_t size,
+				  pgprot_t sect_prot, pgprot_t pte_prot)
 {
 	if (virt < VMALLOC_START) {
 		pr_warn("BUG: not creating mapping for %pa at 0x%016lx - outside kernel range\n",
 			&phys, virt);
 		return;
 	}
-	__create_mapping(pgd_offset_k(virt & PAGE_MASK), phys, virt, size, 0);
+
+	return __create_mapping(pgd_offset_k(virt & PAGE_MASK), phys, virt,
+				size, sect_prot, pte_prot, 0, true);
 }
 
-void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io)
+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)
 {
-	if ((addr >> PGDIR_SHIFT) >= ARRAY_SIZE(idmap_pg_dir)) {
-		pr_warn("BUG: not creating id mapping for %pa\n", &addr);
+	if (virt < VMALLOC_START) {
+		pr_warn("BUG: not creating mapping for %pa at 0x%016lx - outside kernel range\n",
+			&phys, virt);
 		return;
 	}
-	__create_mapping(&idmap_pg_dir[pgd_index(addr)],
-			 addr, addr, size, map_io);
+
+	return __create_mapping(pgd_offset_k(virt & PAGE_MASK), phys, virt,
+				size, sect_prot, pte_prot, 0, false);
+}
+
+#ifdef CONFIG_DEBUG_RODATA
+static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
+{
+	/*
+	 * Set up the executable regions using the existing section mappings
+	 * for 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);
+
+	if (end < kernel_x_start) {
+		create_mapping(start, __phys_to_virt(start),
+			end - start, PROT_SECT_NORMAL, PAGE_KERNEL);
+	} else if (start >= kernel_x_end) {
+		create_mapping(start, __phys_to_virt(start),
+			end - start, PROT_SECT_NORMAL, PAGE_KERNEL);
+	} else {
+		if (start < kernel_x_start)
+			create_mapping(start, __phys_to_virt(start),
+				kernel_x_start - start,
+				PROT_SECT_NORMAL,
+				PAGE_KERNEL);
+		create_mapping(kernel_x_start,
+				__phys_to_virt(kernel_x_start),
+				kernel_x_end - kernel_x_start,
+				PROT_SECT_NORMAL_EXEC, PAGE_KERNEL_EXEC);
+		if (kernel_x_end < end)
+			create_mapping(kernel_x_end,
+				__phys_to_virt(kernel_x_end),
+				end - kernel_x_end,
+				PROT_SECT_NORMAL,
+				PAGE_KERNEL);
+	}
+
+}
+#else
+static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
+{
+	create_mapping(start, __phys_to_virt(start), end - start,
+			PROT_SECT_NORMAL_EXEC, PAGE_KERNEL_EXEC);
 }
+#endif
 
 static void __init map_mem(void)
 {

@@ -328,14 +503,69 @@ static void __init map_mem(void)
 			memblock_set_current_limit(limit);
 		}
 #endif
-
-		create_mapping(start, __phys_to_virt(start), end - start);
+		__map_memblock(start, end);
 	}
 
 	/* Limit no longer required. */
 	memblock_set_current_limit(MEMBLOCK_ALLOC_ANYWHERE);
 }
 
+void __init fixup_executable(void)
+{
+#ifdef CONFIG_DEBUG_RODATA
+	/* 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_NORMAL,
+				PAGE_KERNEL);
+	}
+
+	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_NORMAL,
+				PAGE_KERNEL);
+	}
+#endif
+}
+
+#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_NORMAL_EXEC | PMD_SECT_RDONLY,
+				PAGE_KERNEL_EXEC | 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_NORMAL,
+			PAGE_KERNEL);
+	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.

@@ -345,6 +575,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

Signature · 2 lines

Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Quoted 13 lines · Laura Abbott

Add page protections for arm64 similar to those in arm. This is for security reasons to prevent certain classes of exploits. The current method:

- 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.

Quoted 1 line

Signed-off-by Laura Abbott <[email protected]>

Hi Laura,
I have some comments below.

Quoted 11 lines

---
v4: Combined the Kconfig options

---
 arch/arm64/Kconfig.debug            |  23 +++
 arch/arm64/include/asm/cacheflush.h |   4 +
 arch/arm64/kernel/vmlinux.lds.S     |  17 ++
 arch/arm64/mm/init.c                |   1 +
 arch/arm64/mm/mm.h                  |   2 +
 arch/arm64/mm/mmu.c                 | 303 +++++++++++++++++++++++++++++++-----
 6 files changed, 314 insertions(+), 36 deletions(-)

Quoted 74 lines

arch/arm64/mm/mmu.c +43 -4

index e92f633..0428294 100644
@@ -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/fixmap.h>

@@ -137,17 +138,55 @@ static void __init *early_alloc(unsigned long sz)
      return ptr;
 }
 
-static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
+/*
+ * remap a PMD into pages
+ */
+static noinline void __ref split_pmd(pmd_t *pmd, bool early)
+{
+     pte_t *pte, *start_pte;
+     unsigned long pfn;
+     int i = 0;
+
+     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 = pmd_pfn(*pmd);
+
+     do {
+             /*
+              * Need to have the least restrictive permissions available
+              * permissions will be fixed up later
+              */
+             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)
+                               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, early);
 
      pte = pte_offset_kernel(pmd, addr);
      do {

@@ -156,29 +195,52 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
      } while (pte++, addr += PAGE_SIZE, addr != end);

arch/arm64/mm/mmu.c static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr, · line 204

Quoted 10 lines

arch/arm64/mm/mmu.c +7 -1

 }
 
-static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
+void __ref split_pud(pud_t *old_pud, pmd_t *pmd)
+{
+     unsigned long pfn = pud_pfn(*old_pud);
+        const pmdval_t mask = PMD_SECT_USER | PMD_SECT_PXN | PMD_SECT_UXN |
+                              PMD_SECT_RDONLY | PMD_SECT_PROT_NONE |
+                              PMD_SECT_VALID;
+     pgprot_t prot = __pgprot(pud_val(*old_pud) & mask);

This looks a little fragile, if I've understood things correctly then all we want to do is create a set of pmds with the same pgprot_t as the original pud?

In that case it would probably be easier to simply mask out the pfn from our pud to create the pgprot rather than mask in a set of attributes (that may change in future)?

Quoted 19 lines

arch/arm64/mm/mmu.c +11 -3

+     int i = 0;
+
+     do {
+             set_pmd(pmd, pfn_pmd(pfn, prot));
+             pfn++;
+     } while (pmd++, i++, i < PTRS_PER_PMD);
+}
+
+static void __ref alloc_init_pmd(pud_t *pud, unsigned long addr,
                                unsigned long end, phys_addr_t phys,
-                               int map_io)
+                               pgprot_t sect_prot, pgprot_t pte_prot,
+                               bool early, int map_io)
 {
      pmd_t *pmd;
      unsigned long next;
-     pmdval_t prot_sect;
-     pgprot_t prot_pte;
 

arch/arm64/mm/mmu.c static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr, · line 224

Quoted 9 lines

arch/arm64/mm/mmu.c +2 -5

      if (map_io) {
-             prot_sect = PROT_SECT_DEVICE_nGnRE;
-             prot_pte = __pgprot(PROT_DEVICE_nGnRE);
-     } else {
-             prot_sect = PROT_SECT_NORMAL_EXEC;
-             prot_pte = PAGE_KERNEL_EXEC;
+             sect_prot = PROT_SECT_DEVICE_nGnRE;
+             pte_prot = __pgprot(PROT_DEVICE_nGnRE);
      }

I think we should wipe out map_io from all these functions as it's causing too much complexity. It's enough to have just sect_prot and pte_prot. I posted some similar feedback to Ard:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/296918.html

Quoted 323 lines

arch/arm64/mm/mmu.c +204 -24

      /*
       * 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);
+             if (pud_sect(*pud)) {
+                     /*
+                      * need to have the 1G of mappings continue to be
+                      * present
+                      */
+                     split_pud(pud, pmd);
+             }
              pud_populate(&init_mm, pud, pmd);
+             flush_tlb_all();
      }
 
      pmd = pmd_offset(pud, addr);

@@ -186,8 +248,8 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
              next = pmd_addr_end(addr, end);
              /* try section mapping first */
              if (((addr | next | phys) & ~SECTION_MASK) == 0) {
-                     pmd_t old_pmd =*pmd;
-                     set_pmd(pmd, __pmd(phys | prot_sect));
+                     pmd_t old_pmd = *pmd;
+                     set_pmd(pmd, __pmd(phys | sect_prot));
                      /*
                       * Check for previous table entries created during
                       * boot (__create_page_tables) and flush them.

@@ -196,15 +258,16 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
                              flush_tlb_all();
              } else {
                      alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys),
-                                    prot_pte);
+                                     pte_prot, early);
              }
              phys += next - addr;
      } while (pmd++, addr = next, addr != end);
 }
 
-static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,
+static void __ref alloc_init_pud(pgd_t *pgd, unsigned long addr,
                                unsigned long end, unsigned long phys,
-                               int map_io)
+                               pgprot_t sect_prot, pgprot_t pte_prot,
+                               bool early, int map_io)
 {
      pud_t *pud;
      unsigned long next;

@@ -222,10 +285,10 @@ static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,
              /*
               * For 4K granule only, attempt to put down a 1GB block
               */
-             if (!map_io && (PAGE_SHIFT == 12) &&
+             if (!map_io && early && (PAGE_SHIFT == 12) &&
                  ((addr | next | phys) & ~PUD_MASK) == 0) {
                      pud_t old_pud = *pud;
-                     set_pud(pud, __pud(phys | PROT_SECT_NORMAL_EXEC));
+                     set_pud(pud, __pud(phys | sect_prot));
 
                      /*
                       * If we have an old value for a pud, it will

@@ -240,7 +303,8 @@ static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,
                              flush_tlb_all();
                      }
              } else {
-                     alloc_init_pmd(pud, addr, next, phys, map_io);
+                     alloc_init_pmd(pud, addr, next, phys, sect_prot,
+                                     pte_prot, early, map_io);
              }
              phys += next - addr;
      } while (pud++, addr = next, addr != end);

@@ -250,9 +314,11 @@ 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(pgd_t *pgd, phys_addr_t phys,
-                                 unsigned long virt, phys_addr_t size,
-                                 int map_io)
+static void __ref __create_mapping(pgd_t *pgd, phys_addr_t phys,
+                               unsigned long virt,
+                               phys_addr_t size,
+                               pgprot_t sect_prot, pgprot_t pte_prot,
+                               int map_io, bool early)
 {
      unsigned long addr, length, end, next;
 

@@ -262,31 +328,140 @@ static void __init __create_mapping(pgd_t *pgd, phys_addr_t phys,
      end = addr + length;
      do {
              next = pgd_addr_end(addr, end);
-             alloc_init_pud(pgd, addr, next, phys, map_io);
+             alloc_init_pud(pgd, addr, next, phys, sect_prot, pte_prot,
+                             early, map_io);
              phys += next - addr;
      } while (pgd++, addr = next, addr != end);
 }
 
-static void __init create_mapping(phys_addr_t phys, unsigned long virt,
-                               phys_addr_t size)
+void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io)
+{
+     if ((addr >> PGDIR_SHIFT) >= ARRAY_SIZE(idmap_pg_dir)) {
+             pr_warn("BUG: not creating id mapping for %pa\n", &addr);
+             return;
+     }
+     __create_mapping(&idmap_pg_dir[pgd_index(addr)],
+                      addr, addr, size, PROT_SECT_NORMAL_EXEC,
+                      PAGE_KERNEL_EXEC, map_io, false);
+}
+
+static inline pmd_t *pmd_off_k(unsigned long virt)
+{
+     return pmd_offset(pud_offset(pgd_offset_k(virt), virt), virt);
+}
+
+#ifdef CONFIG_EARLY_PRINTK
+/*
+ * Create an early I/O mapping using the pgd/pmd entries already populated
+ * in head.S as this function is called too early to allocated any memory. The
+ * mapping size is 2MB with 4KB pages or 64KB or 64KB pages.
+ */
+void __iomem * __init early_io_map(phys_addr_t phys, unsigned long virt)
+{
+     unsigned long size, mask;
+     bool page64k = IS_ENABLED(CONFIG_ARM64_64K_PAGES);
+     pgd_t *pgd;
+     pud_t *pud;
+     pmd_t *pmd;
+     pte_t *pte;
+
+     /*
+      * No early pte entries with !ARM64_64K_PAGES configuration, so using
+      * sections (pmd).
+      */
+     size = page64k ? PAGE_SIZE : SECTION_SIZE;
+     mask = ~(size - 1);
+
+     pgd = pgd_offset_k(virt);
+     pud = pud_offset(pgd, virt);
+     if (pud_none(*pud))
+             return NULL;
+     pmd = pmd_offset(pud, virt);
+
+     if (page64k) {
+             if (pmd_none(*pmd))
+                     return NULL;
+             pte = pte_offset_kernel(pmd, virt);
+             set_pte(pte, __pte((phys & mask) | PROT_DEVICE_nGnRE));
+     } else {
+             set_pmd(pmd, __pmd((phys & mask) | PROT_SECT_DEVICE_nGnRE));
+     }
+
+     return (void __iomem *)((virt & mask) + (phys & ~mask));
+}
+#endif
+
+static void __ref create_mapping(phys_addr_t phys, unsigned long virt,
+                               phys_addr_t size,
+                               pgprot_t sect_prot, pgprot_t pte_prot)
 {
      if (virt < VMALLOC_START) {
              pr_warn("BUG: not creating mapping for %pa at 0x%016lx - outside kernel range\n",
                      &phys, virt);
              return;
      }
-     __create_mapping(pgd_offset_k(virt & PAGE_MASK), phys, virt, size, 0);
+
+     return __create_mapping(pgd_offset_k(virt & PAGE_MASK), phys, virt,
+                             size, sect_prot, pte_prot, 0, true);
 }
 
-void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io)
+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)
 {
-     if ((addr >> PGDIR_SHIFT) >= ARRAY_SIZE(idmap_pg_dir)) {
-             pr_warn("BUG: not creating id mapping for %pa\n", &addr);
+     if (virt < VMALLOC_START) {
+             pr_warn("BUG: not creating mapping for %pa at 0x%016lx - outside kernel range\n",
+                     &phys, virt);
              return;
      }
-     __create_mapping(&idmap_pg_dir[pgd_index(addr)],
-                      addr, addr, size, map_io);
+
+     return __create_mapping(pgd_offset_k(virt & PAGE_MASK), phys, virt,
+                             size, sect_prot, pte_prot, 0, false);
+}
+
+#ifdef CONFIG_DEBUG_RODATA
+static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
+{
+     /*
+      * Set up the executable regions using the existing section mappings
+      * for 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);
+
+     if (end < kernel_x_start) {
+             create_mapping(start, __phys_to_virt(start),
+                     end - start, PROT_SECT_NORMAL, PAGE_KERNEL);
+     } else if (start >= kernel_x_end) {
+             create_mapping(start, __phys_to_virt(start),
+                     end - start, PROT_SECT_NORMAL, PAGE_KERNEL);
+     } else {
+             if (start < kernel_x_start)
+                     create_mapping(start, __phys_to_virt(start),
+                             kernel_x_start - start,
+                             PROT_SECT_NORMAL,
+                             PAGE_KERNEL);
+             create_mapping(kernel_x_start,
+                             __phys_to_virt(kernel_x_start),
+                             kernel_x_end - kernel_x_start,
+                             PROT_SECT_NORMAL_EXEC, PAGE_KERNEL_EXEC);
+             if (kernel_x_end < end)
+                     create_mapping(kernel_x_end,
+                             __phys_to_virt(kernel_x_end),
+                             end - kernel_x_end,
+                             PROT_SECT_NORMAL,
+                             PAGE_KERNEL);
+     }
+
+}
+#else
+static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
+{
+     create_mapping(start, __phys_to_virt(start), end - start,
+                     PROT_SECT_NORMAL_EXEC, PAGE_KERNEL_EXEC);
 }
+#endif
 
 static void __init map_mem(void)
 {

@@ -328,14 +503,69 @@ static void __init map_mem(void)
                      memblock_set_current_limit(limit);
              }
 #endif
-
-             create_mapping(start, __phys_to_virt(start), end - start);
+             __map_memblock(start, end);
      }
 
      /* Limit no longer required. */
      memblock_set_current_limit(MEMBLOCK_ALLOC_ANYWHERE);
 }
 
+void __init fixup_executable(void)
+{
+#ifdef CONFIG_DEBUG_RODATA
+     /* 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_NORMAL,
+                             PAGE_KERNEL);
+     }
+
+     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_NORMAL,
+                             PAGE_KERNEL);
+     }
+#endif
+}
+
+#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_NORMAL_EXEC | PMD_SECT_RDONLY,
+                             PAGE_KERNEL_EXEC | 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_NORMAL,
+                     PAGE_KERNEL);
+     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.

@@ -345,6 +575,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
--

Quoted 3 lines

Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 1 line · Steve Capper

On Mon, Oct 27, 2014 at 01:12:32PM -0700, Laura Abbott wrote:

Quoted 6 lines

I think we should wipe out map_io from all these functions as it's causing too much complexity. It's enough to have just sect_prot and pte_prot. I posted some similar feedback to Ard:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/296918.html

I do agree. But could we have a single enum that maps onto {sect_prot, pte_prot} tuples rather than having to pass both values everywhere we call any of these functions?
I.e., { MMU_PROT_DEFAULT, MMU_PROT_READONLY, MMU_PROT_READWRITE, MMU_PROT_READEXEC, MMU_PROT_DEVICE }, with the mapping local to mmu.c?

Quoted 13 lines · Laura Abbott

Add page protections for arm64 similar to those in arm. This is for security reasons to prevent certain classes of exploits. The current method:

- 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.

Quoted 1 line

Signed-off-by Laura Abbott <[email protected]>

Hi Laura,
I have some comments below.

arch/arm64/mm/mmu.c static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr, · line 204

Quoted 12 lines

arch/arm64/mm/mmu.c +7 -1

@@ -156,29 +195,52 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
  	} while (pte++, addr += PAGE_SIZE, addr != end);
  }
 
-static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
+void __ref split_pud(pud_t *old_pud, pmd_t *pmd)
+{
+	unsigned long pfn = pud_pfn(*old_pud);
+        const pmdval_t mask = PMD_SECT_USER | PMD_SECT_PXN | PMD_SECT_UXN |
+                              PMD_SECT_RDONLY | PMD_SECT_PROT_NONE |
+                              PMD_SECT_VALID;
+	pgprot_t prot = __pgprot(pud_val(*old_pud) & mask);

This looks a little fragile, if I've understood things correctly then all we want to do is create a set of pmds with the same pgprot_t as the original pud?

In that case it would probably be easier to simply mask out the pfn from our pud to create the pgprot rather than mask in a set of attributes (that may change in future)?

Quoted 19 lines

arch/arm64/mm/mmu.c +11 -3

+	int i = 0;
+
+	do {
+		set_pmd(pmd, pfn_pmd(pfn, prot));
+		pfn++;
+	} while (pmd++, i++, i < PTRS_PER_PMD);
+}
+
+static void __ref alloc_init_pmd(pud_t *pud, unsigned long addr,
  				  unsigned long end, phys_addr_t phys,
-				  int map_io)
+				  pgprot_t sect_prot, pgprot_t pte_prot,
+				  bool early, int map_io)
  {
  	pmd_t *pmd;
  	unsigned long next;
-	pmdval_t prot_sect;
-	pgprot_t prot_pte;
 

arch/arm64/mm/mmu.c static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr, · line 224

Quoted 9 lines

arch/arm64/mm/mmu.c +2 -5

  	if (map_io) {
-		prot_sect = PROT_SECT_DEVICE_nGnRE;
-		prot_pte = __pgprot(PROT_DEVICE_nGnRE);
-	} else {
-		prot_sect = PROT_SECT_NORMAL_EXEC;
-		prot_pte = PAGE_KERNEL_EXEC;
+		sect_prot = PROT_SECT_DEVICE_nGnRE;
+		pte_prot = __pgprot(PROT_DEVICE_nGnRE);
  	}

I think we should wipe out map_io from all these functions as it's causing too much complexity. It's enough to have just sect_prot and pte_prot. I posted some similar feedback to Ard:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/296918.html

Add page protections for arm64 similar to those in arm. This is for security reasons to prevent certain classes of exploits. The current method:

- 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 <[email protected]>

---
v4: Combined the Kconfig options

---
 arch/arm64/Kconfig.debug            |  23 +++
 arch/arm64/include/asm/cacheflush.h |   4 +
 arch/arm64/kernel/vmlinux.lds.S     |  17 ++
 arch/arm64/mm/init.c                |   1 +
 arch/arm64/mm/mm.h                  |   2 +
 arch/arm64/mm/mmu.c                 | 303 +++++++++++++++++++++++++++++++-----
 6 files changed, 314 insertions(+), 36 deletions(-)

[resending due to previous header destruction]

Hi Laura,

Quoted 16 lines · Laura Abbott

Add page protections for arm64 similar to those in arm. This is for security reasons to prevent certain classes of exploits. The current method:

- 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 <[email protected]>

---
v4: Combined the Kconfig options

Quoted 8 lines

---
  arch/arm64/Kconfig.debug            |  23 +++
  arch/arm64/include/asm/cacheflush.h |   4 +
  arch/arm64/kernel/vmlinux.lds.S     |  17 ++
  arch/arm64/mm/init.c                |   1 +
  arch/arm64/mm/mm.h                  |   2 +
  arch/arm64/mm/mmu.c                 | 303 +++++++++++++++++++++++++++++++-----
  6 files changed, 314 insertions(+), 36 deletions(-)

With this patch applied to v3.18-rc2, my board to blows up at boot when using UEFI (without DEBUG_RODATA selected):

---->8----
EFI stub: Booting Linux Kernel...
Initializing cgroup subsys cpu
Linux version 3.18.0-rc2+ (mark at leverpostej) (gcc version 4.9.2 20140904 (prerelease) (crosstool-NG linaro-1.13.1-4.9-2014.09 - Linaro GCC 4.9-2014.09) ) #112 SMP PREEMPT Tue Oct 28 13:58:41 GMT 2014
CPU: AArch64 Processor [410fd030] revision 0
Detected VIPT I-cache on CPU0
bootconsole [uart0] enabled
efi: Getting EFI parameters from FDT:
EFI v2.40 by ARM Juno EFI Oct 7 2014 15:05:42
efi: ACPI=0xfebdc000 ACPI 2.0=0xfebdc014
cma: Reserved 16 MiB at fd800000

BUG: failure at arch/arm64/mm/mmu.c:234/alloc_init_pmd()!
Kernel panic - not syncing: BUG!
CPU: 0 PID: 0 Comm: swapper Not tainted 3.18.0-rc2+ #112
Call trace:
[<ffffffc000087ec8>] dump_backtrace+0x0/0x124
[<ffffffc000087ffc>] show_stack+0x10/0x1c
[<ffffffc0004ebd58>] dump_stack+0x74/0xb8
[<ffffffc0004eb018>] panic+0xe0/0x220
[<ffffffc0004e8e08>] alloc_init_pmd+0x1cc/0x1dc
[<ffffffc0004e8e3c>] alloc_init_pud+0x24/0x6c
[<ffffffc0004e8f54>] __create_mapping+0xd0/0xf0
[<ffffffc00069a0a0>] create_id_mapping+0x5c/0x68
[<ffffffc00069964c>] efi_idmap_init+0x54/0xd8
[<ffffffc0006978a8>] setup_arch+0x408/0x5c0
[<ffffffc00069566c>] start_kernel+0x94/0x3a0
---[ end Kernel panic - not syncing: BUG!

---->8----

I've not yet figured out precisely why. I haven't tried a !EFI boot because of the way my board is set up at the moment.

Hi,

This is v4 of the series to add stricter page protections for arm64. The goal is to have text be RO/X and everything else be RW/NX. More testing is still needed all around, especially with additional page table levels. Some of this work may conflict with ongoing work to support ARCH_DEBUG_PAGEALLOC on arm64[1] so depending on where that goes I may need to rebase or re-work the code.

Thanks,
Laura

[1] http://www.spinics.net/lists/arm-kernel/msg372216.html

Laura Abbott (7):
arm64: Treat handle_arch_irq as a function pointer
arm64: Switch to ldr for loading the stub vectors
arm64: Move cpu_resume into the text section
arm64: Move some head.text functions to executable section
arm64: Factor out fixmap initialiation from ioremap
arm64: use fixmap for text patching when text is RO
arm64: add better page protections to arm64

  arch/arm64/Kconfig.debug            |  23 +++
  arch/arm64/include/asm/cacheflush.h |   4 +
  arch/arm64/include/asm/fixmap.h     |   8 +-
  arch/arm64/include/asm/insn.h       |   2 +
  arch/arm64/include/asm/irq.h        |   1 -
  arch/arm64/kernel/entry.S           |   6 +-
  arch/arm64/kernel/head.S            |   7 +-
  arch/arm64/kernel/insn.c            |  72 ++++++-
  arch/arm64/kernel/irq.c             |   2 +
  arch/arm64/kernel/jump_label.c      |   2 +-
  arch/arm64/kernel/setup.c           |   3 +-
  arch/arm64/kernel/sleep.S           |  12 +-
  arch/arm64/kernel/vmlinux.lds.S     |  17 ++
  arch/arm64/mm/init.c                |   1 +
  arch/arm64/mm/ioremap.c             |  93 +--------
  arch/arm64/mm/mm.h                  |   2 +
  arch/arm64/mm/mmu.c                 | 397 ++++++++++++++++++++++++++++++++----
  17 files changed, 507 insertions(+), 145 deletions(-)