Re: [PATCH v2] ARM: mm: mark section-aligned portion of rodata NX

[Ard Biesheuvel]

On 22 January 2016 at 22:19, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Tue, Dec 8, 2015 at 10:38 AM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>> On Mon, Dec 7, 2015 at 11:47 PM, Ard Biesheuvel
>> <ard.biesheuvel@xxxxxxxxxx> wrote:
>>> On 7 December 2015 at 23:35, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>>>> When rodata is large enough that it crosses a section boundary after the
>>>> kernel text, mark the rest NX. This is as close to full NX of rodata as
>>>> we can get without splitting page tables or doing section alignment via
>>>> CONFIG_DEBUG_ALIGN_RODATA.
>>>>
>>>> When the config is:
>>>>
>>>>  CONFIG_DEBUG_RODATA=y
>>>>  # CONFIG_DEBUG_ALIGN_RODATA is not set
>>>>
>>>> Before:
>>>>
>>>> ---[ Kernel Mapping ]---
>>>> 0x80000000-0x80100000           1M     RW NX SHD
>>>> 0x80100000-0x80a00000           9M     ro x  SHD
>>>> 0x80a00000-0xa0000000         502M     RW NX SHD
>>>>
>>>> After:
>>>>
>>>> ---[ Kernel Mapping ]---
>>>> 0x80000000-0x80100000           1M     RW NX SHD
>>>> 0x80100000-0x80700000           6M     ro x  SHD
>>>> 0x80700000-0x80a00000           3M     ro NX SHD
>>>> 0x80a00000-0xa0000000         502M     RW NX SHD
>>>>
>>>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>>>> ---
>>>> v2:
>>>> - static declaration, ard
>>>> ---
>>>>  arch/arm/kernel/vmlinux.lds.S | 9 +++++++--
>>>>  arch/arm/mm/init.c            | 7 ++++---
>>>>  2 files changed, 11 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
>>>> index a6e395c53a48..9c249c71fda1 100644
>>>> --- a/arch/arm/kernel/vmlinux.lds.S
>>>> +++ b/arch/arm/kernel/vmlinux.lds.S
>>>> @@ -8,9 +8,7 @@
>>>>  #include <asm/thread_info.h>
>>>>  #include <asm/memory.h>
>>>>  #include <asm/page.h>
>>>> -#ifdef CONFIG_DEBUG_RODATA
>>>>  #include <asm/pgtable.h>
>>>> -#endif
>>>>
>>>>  #define PROC_INFO                                                      \
>>>>         . = ALIGN(4);                                                   \
>>>> @@ -337,6 +335,13 @@ SECTIONS
>>>>  }
>>>>
>>>>  /*
>>>> + * Without CONFIG_DEBUG_ALIGN_RODATA, __start_rodata_section_aligned will
>>>> + * be the first section-aligned location after __start_rodata. Otherwise,
>>>> + * it will be equal to __start_rodata.
>>>> + */
>>>> +__start_rodata_section_aligned = ALIGN(__start_rodata, 1 << SECTION_SHIFT);
>>>> +
>>>> +/*
>>>>   * These must never be empty
>>>>   * If you have to comment these two assert statements out, your
>>>>   * binutils is too old (for other reasons as well)
>>>> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>>>> index 321d3683dc7c..6b16f6cf4843 100644
>>>> --- a/arch/arm/mm/init.c
>>>> +++ b/arch/arm/mm/init.c
>>>> @@ -579,6 +579,9 @@ struct section_perm {
>>>>         pmdval_t clear;
>>>>  };
>>>>
>>>> +/* First section-aligned location at or after __start_rodata. */
>>>> +extern char __start_rodata_section_aligned[];
>>>> +
>>>>  static struct section_perm nx_perms[] = {
>>>>         /* Make pages tables, etc before _stext RW (set NX). */
>>>>         {
>>>> @@ -596,16 +599,14 @@ static struct section_perm nx_perms[] = {
>>>>                 .mask   = ~PMD_SECT_XN,
>>>>                 .prot   = PMD_SECT_XN,
>>>>         },
>>>> -#ifdef CONFIG_DEBUG_ALIGN_RODATA
>>>>         /* Make rodata NX (set RO in ro_perms below). */
>>>>         {
>>>>                 .name   = "rodata NX",
>>>> -               .start  = (unsigned long)__start_rodata,
>>>> +               .start  = (unsigned long)__start_rodata_section_aligned,
>>>>                 .end    = (unsigned long)__init_begin,
>>>
>>> What happens if start > end ?
>>
>> It isn't possible, since rodata will be after text and before
>> init_begin, so it must either less than or equal to init_begin. The
>> equal case is quite possible, and that's fine, since both cases are
>> (correctly) silently ignored by set_section_perms:
>>
>>                 for (addr = perms[i].start; addr < perms[i].end; addr
>> += SECTION_SIZE)
>>                     section_update(....
>>
>> -Kees
>
> Hi Ard,
>
> Is this v2 ok? I'd like to have your Ack before sending it to the patch tracker.
>

Looks fine to me

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>

Thanks,
Ard.