| Message ID | 1408584039-12735-4-git-send-email-lauraa@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/20/14 18:20, Laura Abbott wrote: > The function cpu_resume currently lives in the .data > section. This breaks functionality to make .data no > execute. Move cpu_resume to .text which is where it > actually belongs. > > Signed-off-by: Laura Abbott <lauraa@codeaurora.org> > --- This is similar to arm (where I don't see a similar patch to this in Kees' patchset for RO). In arm we have this comment: /* * Note: Yes, part of the following code is located into the .data section. * This is to allow sleep_save_sp to be accessed with a relative load * while we can't rely on any MMU translation. We could have put * sleep_save_sp in the .text section as well, but some setups might * insist on it to be truly read-only. */ Doesn't the same situation apply here? How is this problem overcome? But I'm a little confused, shouldn't this code run with the MMU disabled? > ENDPROC(cpu_resume_after_mmu) > > - .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 > ldr x5, [x4] > add x8, x4, x5 // x8 = struct mpidr_hash phys address > /* retrieve mpidr_hash members to compute the hash */ > @@ -143,14 +143,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] > 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 > @@ -160,6 +161,7 @@ ENTRY(cpu_resume) > b cpu_resume_mmu // Resume MMU, never returns > ENDPROC(cpu_resume) > > + .data > .align 3 > mpidr_hash_ptr: > /*
On 8/25/2014 1:34 PM, Stephen Boyd wrote: > On 08/20/14 18:20, Laura Abbott wrote: >> The function cpu_resume currently lives in the .data >> section. This breaks functionality to make .data no >> execute. Move cpu_resume to .text which is where it >> actually belongs. >> >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org> >> --- > > This is similar to arm (where I don't see a similar patch to this in > Kees' patchset for RO). In arm we have this comment: > > /* > * Note: Yes, part of the following code is located into the .data section. > * This is to allow sleep_save_sp to be accessed with a relative load > * while we can't rely on any MMU translation. We could have put > * sleep_save_sp in the .text section as well, but some setups might > * insist on it to be truly read-only. > */ > > Doesn't the same situation apply here? How is this problem overcome? > > But I'm a little confused, shouldn't this code run with the MMU disabled? > >> ENDPROC(cpu_resume_after_mmu) >> >> - .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 >> ldr x5, [x4] >> add x8, x4, x5 // x8 = struct mpidr_hash phys address >> /* retrieve mpidr_hash members to compute the hash */ >> @@ -143,14 +143,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] >> 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 >> @@ -160,6 +161,7 @@ ENTRY(cpu_resume) >> b cpu_resume_mmu // Resume MMU, never returns >> ENDPROC(cpu_resume) >> >> + .data >> .align 3 >> mpidr_hash_ptr: >> /* > > Good point. I think this was a patch I added when I was debugging other issues and assumed it would be needed (code in .data segment, seems naturally a problem, right?) . When I revert the patch though it seems to work just fine. I suspect the comment about pc relative load is no longer relevant since I use the relocation trick to properly access sleep_save_sp in the data section. Since it's not technically needed, I could drop the patch and add one adding the comment back saying this was done on purpose. On the other hand, I wonder if I could do something 'interesting' by modifying the cpu_resume code since it's writable if I was a malicious program. Thanks, Laura
On 08/25/14 17:43, Laura Abbott wrote: > Good point. I think this was a patch I added when I was debugging other > issues and assumed it would be needed (code in .data segment, seems > naturally a problem, right?) . When I revert the patch though it seems > to work just fine. I suspect the comment about pc relative load is no > longer relevant since I use the relocation trick to properly access > sleep_save_sp in the data section. Ah good. Can we move this code to the text section on arm32 as well please? Probably update the commit text too. > > Since it's not technically needed, I could drop the patch and add one > adding the comment back saying this was done on purpose. On the other > hand, I wonder if I could do something 'interesting' by modifying > the cpu_resume code since it's writable if I was a malicious > program. Even moving the cpu_resume function into the text section doesn't prevent a malicious program which can write to the sleep_save_sp area to use a different resume function. I suppose that is a bit harder to do though.
diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S index b192572..6ee13db 100644 --- a/arch/arm64/kernel/sleep.S +++ b/arch/arm64/kernel/sleep.S @@ -126,12 +126,12 @@ cpu_resume_after_mmu: ret ENDPROC(cpu_resume_after_mmu) - .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 ldr x5, [x4] add x8, x4, x5 // x8 = struct mpidr_hash phys address /* retrieve mpidr_hash members to compute the hash */ @@ -143,14 +143,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] 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 @@ -160,6 +161,7 @@ ENTRY(cpu_resume) b cpu_resume_mmu // Resume MMU, never returns ENDPROC(cpu_resume) + .data .align 3 mpidr_hash_ptr: /*
The function cpu_resume currently lives in the .data section. This breaks functionality to make .data no execute. Move cpu_resume to .text which is where it actually belongs. Signed-off-by: Laura Abbott <lauraa@codeaurora.org> --- arch/arm64/kernel/sleep.S | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)