[PATCHv3,4/7] arm64: Move some head.text functions to executable section

Commit Message

Laura Abbott Aug. 21, 2014, 9:42 p.m. UTC

On 8/21/2014 3:34 AM, Mark Rutland wrote:
> Hi Laura,
> 
> On Thu, Aug 21, 2014 at 02:20:36AM +0100, Laura Abbott wrote:
>> The code in the head.text section of the kernel exists in the
>> same section as the swapper_pg_dir which means it needs the
>> same page table permissions. The swapper_pg_dir needs to be
>> writeable but shouldn't be executable.
> 
> I think we can drop the above. As far as I can tell as of commit
> bd00cd5f8c8c (arm64: place initial page tables above the kernel) it's no
> longer relevant.
>

Yes, this should be changed. Instead of citing swapper_pg_dir, I need
to cite the fact that there may still be memory outside of stext which
will get freed to the buddy allocator and therefore should be RW/NX.
 
>> The head.text section
>> is intended to be run at early bootup before any of the regular
>> kernel mappings have been setup so there is no issue at bootup.
>> The suspend/resume/hotplug code path requires some of these
>> head.S functions to run however which means they need to be
>> executable. We can't easily move all of the head.text to
>> an executable section, so split it into two parts: that which
>> is used only at early head.S bootup and that which is used
>> after bootup. There is a small bit of code duplication because
>> of some relocation issues related to accessing code more than
>> 1MB away.
> 
> From a cursory glance it looks like the only things we need write access
> to in .head.text are __cpu_boot_mode and __switch_data. Can't we instead
> place those in .data and make .head.text executable?
> 
> We currently find them with adr, which should be easy to replace with
> adrp + add to get around relocation issues.
> 

__boot_cpu_mode should be placed in data with a push section and
__switch_data is only modified before the permissions are set up.
I took a closer look at the code and the only thing that actually
needs to be executable from head.S is __secondary_switched so
the following patch should be sufficient to cover it:


I think I was a bit over zealous in determining that everything needed
to be placed in .text vs. not.

Thanks,
Laura

Comments

Mark Rutland Aug. 22, 2014, 9:48 a.m. UTC | #1

On Thu, Aug 21, 2014 at 10:42:13PM +0100, Laura Abbott wrote:
> On 8/21/2014 3:34 AM, Mark Rutland wrote:
> > Hi Laura,
> > 
> > On Thu, Aug 21, 2014 at 02:20:36AM +0100, Laura Abbott wrote:
> >> The code in the head.text section of the kernel exists in the
> >> same section as the swapper_pg_dir which means it needs the
> >> same page table permissions. The swapper_pg_dir needs to be
> >> writeable but shouldn't be executable.
> > 
> > I think we can drop the above. As far as I can tell as of commit
> > bd00cd5f8c8c (arm64: place initial page tables above the kernel) it's no
> > longer relevant.
> >
> 
> Yes, this should be changed. Instead of citing swapper_pg_dir, I need
> to cite the fact that there may still be memory outside of stext which
> will get freed to the buddy allocator and therefore should be RW/NX.

Ok. I must be missing something here; we don't seem to free any of
.head.text to the buddy allocator at the moment. Are we just trying to
have the bare minimum remain executable regardless?

> >> The head.text section
> >> is intended to be run at early bootup before any of the regular
> >> kernel mappings have been setup so there is no issue at bootup.
> >> The suspend/resume/hotplug code path requires some of these
> >> head.S functions to run however which means they need to be
> >> executable. We can't easily move all of the head.text to
> >> an executable section, so split it into two parts: that which
> >> is used only at early head.S bootup and that which is used
> >> after bootup. There is a small bit of code duplication because
> >> of some relocation issues related to accessing code more than
> >> 1MB away.
> > 
> > From a cursory glance it looks like the only things we need write access
> > to in .head.text are __cpu_boot_mode and __switch_data. Can't we instead
> > place those in .data and make .head.text executable?
> > 
> > We currently find them with adr, which should be easy to replace with
> > adrp + add to get around relocation issues.
> > 
> 
> __boot_cpu_mode should be placed in data with a push section and
> __switch_data is only modified before the permissions are set up.

Ok, so those are fine then.

> I took a closer look at the code and the only thing that actually
> needs to be executable from head.S is __secondary_switched so
> the following patch should be sufficient to cover it:
>
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index caa9557..5c17599 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -414,12 +414,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 */

If we need to cover __secondary_switched, don't we need to cover
__turn_mmu_on as well, given that will turn the MMU on and branch to
__secondary_switched?

Thanks,
Mark.

Laura Abbott Aug. 26, 2014, 12:32 a.m. UTC | #2

On 8/22/2014 2:48 AM, Mark Rutland wrote:
> On Thu, Aug 21, 2014 at 10:42:13PM +0100, Laura Abbott wrote:
>> On 8/21/2014 3:34 AM, Mark Rutland wrote:
>>> Hi Laura,
>>>
>>> On Thu, Aug 21, 2014 at 02:20:36AM +0100, Laura Abbott wrote:
>>>> The code in the head.text section of the kernel exists in the
>>>> same section as the swapper_pg_dir which means it needs the
>>>> same page table permissions. The swapper_pg_dir needs to be
>>>> writeable but shouldn't be executable.
>>>
>>> I think we can drop the above. As far as I can tell as of commit
>>> bd00cd5f8c8c (arm64: place initial page tables above the kernel) it's no
>>> longer relevant.
>>>
>>
>> Yes, this should be changed. Instead of citing swapper_pg_dir, I need
>> to cite the fact that there may still be memory outside of stext which
>> will get freed to the buddy allocator and therefore should be RW/NX.
> 
> Ok. I must be missing something here; we don't seem to free any of
> .head.text to the buddy allocator at the moment. Are we just trying to
> have the bare minimum remain executable regardless?
> 

It's not head.text per se, but we do 

memblock_reserve(__pa(_text), _end - _text);

to reserve the text area. On the system I was testing on, _text did not
start at PAGE_OFFSET so there was still some not reserved memory
between PAGE_OFFSET and _text which was freed to the buddy allocator.

>>>> The head.text section
>>>> is intended to be run at early bootup before any of the regular
>>>> kernel mappings have been setup so there is no issue at bootup.
>>>> The suspend/resume/hotplug code path requires some of these
>>>> head.S functions to run however which means they need to be
>>>> executable. We can't easily move all of the head.text to
>>>> an executable section, so split it into two parts: that which
>>>> is used only at early head.S bootup and that which is used
>>>> after bootup. There is a small bit of code duplication because
>>>> of some relocation issues related to accessing code more than
>>>> 1MB away.
>>>
>>> From a cursory glance it looks like the only things we need write access
>>> to in .head.text are __cpu_boot_mode and __switch_data. Can't we instead
>>> place those in .data and make .head.text executable?
>>>
>>> We currently find them with adr, which should be easy to replace with
>>> adrp + add to get around relocation issues.
>>>
>>
>> __boot_cpu_mode should be placed in data with a push section and
>> __switch_data is only modified before the permissions are set up.
> 
> Ok, so those are fine then.
> 
>> I took a closer look at the code and the only thing that actually
>> needs to be executable from head.S is __secondary_switched so
>> the following patch should be sufficient to cover it:
>>
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index caa9557..5c17599 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -414,12 +414,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 */
> 
> If we need to cover __secondary_switched, don't we need to cover
> __turn_mmu_on as well, given that will turn the MMU on and branch to
> __secondary_switched?
> 

You should be right but when I was testing this I seemed to get away
without needing it. It's probably just luck and I should move
__turn_mmu_on to text just to be on the safe side as well.

Thanks,
Laura

Mark Rutland Aug. 26, 2014, 5:45 p.m. UTC | #3

On Tue, Aug 26, 2014 at 01:32:27AM +0100, Laura Abbott wrote:
> On 8/22/2014 2:48 AM, Mark Rutland wrote:
> > On Thu, Aug 21, 2014 at 10:42:13PM +0100, Laura Abbott wrote:
> >> On 8/21/2014 3:34 AM, Mark Rutland wrote:
> >>> Hi Laura,
> >>>
> >>> On Thu, Aug 21, 2014 at 02:20:36AM +0100, Laura Abbott wrote:
> >>>> The code in the head.text section of the kernel exists in the
> >>>> same section as the swapper_pg_dir which means it needs the
> >>>> same page table permissions. The swapper_pg_dir needs to be
> >>>> writeable but shouldn't be executable.
> >>>
> >>> I think we can drop the above. As far as I can tell as of commit
> >>> bd00cd5f8c8c (arm64: place initial page tables above the kernel) it's no
> >>> longer relevant.
> >>>
> >>
> >> Yes, this should be changed. Instead of citing swapper_pg_dir, I need
> >> to cite the fact that there may still be memory outside of stext which
> >> will get freed to the buddy allocator and therefore should be RW/NX.
> > 
> > Ok. I must be missing something here; we don't seem to free any of
> > .head.text to the buddy allocator at the moment. Are we just trying to
> > have the bare minimum remain executable regardless?
> > 
> 
> It's not head.text per se, but we do 
> 
> memblock_reserve(__pa(_text), _end - _text);
> 
> to reserve the text area. On the system I was testing on, _text did not
> start at PAGE_OFFSET so there was still some not reserved memory
> between PAGE_OFFSET and _text which was freed to the buddy allocator.

Ah, I see. I'd forgotten about the TEXT_OFFSET area.

Thanks for the clarification.

> >>>> The head.text section
> >>>> is intended to be run at early bootup before any of the regular
> >>>> kernel mappings have been setup so there is no issue at bootup.
> >>>> The suspend/resume/hotplug code path requires some of these
> >>>> head.S functions to run however which means they need to be
> >>>> executable. We can't easily move all of the head.text to
> >>>> an executable section, so split it into two parts: that which
> >>>> is used only at early head.S bootup and that which is used
> >>>> after bootup. There is a small bit of code duplication because
> >>>> of some relocation issues related to accessing code more than
> >>>> 1MB away.
> >>>
> >>> From a cursory glance it looks like the only things we need write access
> >>> to in .head.text are __cpu_boot_mode and __switch_data. Can't we instead
> >>> place those in .data and make .head.text executable?
> >>>
> >>> We currently find them with adr, which should be easy to replace with
> >>> adrp + add to get around relocation issues.
> >>>
> >>
> >> __boot_cpu_mode should be placed in data with a push section and
> >> __switch_data is only modified before the permissions are set up.
> > 
> > Ok, so those are fine then.
> > 
> >> I took a closer look at the code and the only thing that actually
> >> needs to be executable from head.S is __secondary_switched so
> >> the following patch should be sufficient to cover it:
> >>
> >> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> >> index caa9557..5c17599 100644
> >> --- a/arch/arm64/kernel/head.S
> >> +++ b/arch/arm64/kernel/head.S
> >> @@ -414,12 +414,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 */
> > 
> > If we need to cover __secondary_switched, don't we need to cover
> > __turn_mmu_on as well, given that will turn the MMU on and branch to
> > __secondary_switched?
> > 
> 
> You should be right but when I was testing this I seemed to get away
> without needing it. It's probably just luck and I should move
> __turn_mmu_on to text just to be on the safe side as well.

Interesting. I guess the I-cache was on already, then?

Moving __turn_mmu_on to the text section sounds like the right thing to
do.

Cheers,
Mark.

Patch

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index caa9557..5c17599 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -414,12 +414,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 */
 
 /*