| Message ID | 20220613144550.3760857-23-ardb@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | arm64: refactor boot flow and add support for WXN | expand |
On Mon, Jun 13, 2022 at 04:45:46PM +0200, Ard Biesheuvel wrote: > Currently, the ro_after_init sections sits right in the middle of the > text/rodata/inittext segment, making it difficult to map any of those > non-writable during early boot. So instead, move it to the start of > .data, and update the init sequences so that the section is remapped > read-only once startup completes. > > Note that this moves the entire HYP data section into .data as well - > this likely needs to remain as a single block for now, but could perhaps > split into a .rodata and .data..ro_after_init section later. If I'm reading this correctly, this means that .data..ro_after_init now lives between .data and .rodata? Do the various LKDTM tests still pass after this change? Reviewed-by: Kees Cook <keescook@chromium.org>
On Mon, 13 Jun 2022 at 19:00, Kees Cook <keescook@chromium.org> wrote: > > On Mon, Jun 13, 2022 at 04:45:46PM +0200, Ard Biesheuvel wrote: > > Currently, the ro_after_init sections sits right in the middle of the > > text/rodata/inittext segment, making it difficult to map any of those > > non-writable during early boot. So instead, move it to the start of > > .data, and update the init sequences so that the section is remapped > > read-only once startup completes. > > > > Note that this moves the entire HYP data section into .data as well - > > this likely needs to remain as a single block for now, but could perhaps > > split into a .rodata and .data..ro_after_init section later. > > If I'm reading this correctly, this means that .data..ro_after_init now > lives between .data and .rodata? > No, between .initdata and .data > Do the various LKDTM tests still pass after this change? > Good question, I'll check. > Reviewed-by: Kees Cook <keescook@chromium.org> > > -- > Kees Cook
On Mon, Jun 13, 2022 at 07:16:15PM +0200, Ard Biesheuvel wrote: > On Mon, 13 Jun 2022 at 19:00, Kees Cook <keescook@chromium.org> wrote: > > > > On Mon, Jun 13, 2022 at 04:45:46PM +0200, Ard Biesheuvel wrote: > > > Currently, the ro_after_init sections sits right in the middle of the > > > text/rodata/inittext segment, making it difficult to map any of those > > > non-writable during early boot. So instead, move it to the start of > > > .data, and update the init sequences so that the section is remapped > > > read-only once startup completes. > > > > > > Note that this moves the entire HYP data section into .data as well - > > > this likely needs to remain as a single block for now, but could perhaps > > > split into a .rodata and .data..ro_after_init section later. > > > > If I'm reading this correctly, this means that .data..ro_after_init now > > lives between .data and .rodata? > > > > No, between .initdata and .data Ah, doesn't this mean more padding (for segment alignment) used? On other architectures .data..ro_after_init tried to be near the writable/read-only boundary so segment padding was only needed on one side (e.g. it could live at the end of .rodata without segment alignment but before .data which was segment aligned.) Then when .rodata was made read-only (after __init), .data..ro_after_init would also get set read-only. In this case, I think it ends up needing segment alignment both at the front and the end, since the .initdata and .data are freed and left writable, respectively?
On Tue, 14 Jun 2022 at 01:38, Kees Cook <keescook@chromium.org> wrote: > > On Mon, Jun 13, 2022 at 07:16:15PM +0200, Ard Biesheuvel wrote: > > On Mon, 13 Jun 2022 at 19:00, Kees Cook <keescook@chromium.org> wrote: > > > > > > On Mon, Jun 13, 2022 at 04:45:46PM +0200, Ard Biesheuvel wrote: > > > > Currently, the ro_after_init sections sits right in the middle of the > > > > text/rodata/inittext segment, making it difficult to map any of those > > > > non-writable during early boot. So instead, move it to the start of > > > > .data, and update the init sequences so that the section is remapped > > > > read-only once startup completes. > > > > > > > > Note that this moves the entire HYP data section into .data as well - > > > > this likely needs to remain as a single block for now, but could perhaps > > > > split into a .rodata and .data..ro_after_init section later. > > > > > > If I'm reading this correctly, this means that .data..ro_after_init now > > > lives between .data and .rodata? > > > > > > > No, between .initdata and .data > > Ah, doesn't this mean more padding (for segment alignment) used? On other > architectures .data..ro_after_init tried to be near the writable/read-only > boundary so segment padding was only needed on one side (e.g. it could > live at the end of .rodata without segment alignment but before .data > which was segment aligned.) Then when .rodata was made read-only (after > __init), .data..ro_after_init would also get set read-only. > > In this case, I think it ends up needing segment alignment both at the > front and the end, since the .initdata and .data are freed and left > writable, respectively? > We used to have text -- rodata (ro_after_init) -- inittext -- initdata -- data bss where -- are the segment boundaries, which are always aligned to 64k on arm64 After this patch, we get text -- rodata -- inittext -- initdata -- (ro_after_init) data bss so in terms of padding due to alignment, there is not a lot of difference. The main difference here is the fact that we lose the ability to use block mappings, but if anyone cares about that, we could work around this by creating a separate segment for ro_after_init.
On Thu, Jun 16, 2022 at 01:31:23PM +0200, Ard Biesheuvel wrote: > We used to have > > text > -- > rodata > (ro_after_init) > -- > inittext > -- > initdata > -- > data > bss > > where -- are the segment boundaries, which are always aligned to 64k on arm64 > > After this patch, we get > > text > -- > rodata > -- > inittext > -- > initdata > -- > (ro_after_init) > data > bss > > so in terms of padding due to alignment, there is not a lot of difference. But how is ro_after_init read-only and data isn't, if there isn't a segment alignment to make that work out?
On Thu, 16 Jun 2022 at 18:18, Kees Cook <keescook@chromium.org> wrote: > > On Thu, Jun 16, 2022 at 01:31:23PM +0200, Ard Biesheuvel wrote: > > We used to have > > > > text > > -- > > rodata > > (ro_after_init) > > -- > > inittext > > -- > > initdata > > -- > > data > > bss > > > > where -- are the segment boundaries, which are always aligned to 64k on arm64 > > > > After this patch, we get > > > > text > > -- > > rodata > > -- > > inittext > > -- > > initdata > > -- > > (ro_after_init) > > data > > bss > > > > so in terms of padding due to alignment, there is not a lot of difference. > > But how is ro_after_init read-only and data isn't, if there isn't a > segment alignment to make that work out? > Actually, there is a segment alignment between ro_after_init and data - my diagram is inaccurate. But we don't actually need that to remap this slice of memory r/o
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S index 45131e354e27..736aca63dad1 100644 --- a/arch/arm64/kernel/vmlinux.lds.S +++ b/arch/arm64/kernel/vmlinux.lds.S @@ -59,6 +59,7 @@ #define RO_EXCEPTION_TABLE_ALIGN 4 #define RUNTIME_DISCARD_EXIT +#define RO_AFTER_INIT_DATA #include <asm-generic/vmlinux.lds.h> #include <asm/cache.h> @@ -188,30 +189,13 @@ SECTIONS /* everything from this point to __init_begin will be marked RO NX */ RO_DATA(PAGE_SIZE) - HYPERVISOR_DATA_SECTIONS - /* code sections that are never executed via the kernel mapping */ .rodata.text : { TRAMP_TEXT HIBERNATE_TEXT KEXEC_TEXT - . = ALIGN(PAGE_SIZE); } - idmap_pg_dir = .; - . += PAGE_SIZE; - -#ifdef CONFIG_UNMAP_KERNEL_AT_EL0 - tramp_pg_dir = .; - . += PAGE_SIZE; -#endif - - reserved_pg_dir = .; - . += PAGE_SIZE; - - swapper_pg_dir = .; - . += PAGE_SIZE; - . = ALIGN(SEGMENT_ALIGN); __init_begin = .; __inittext_begin = .; @@ -274,6 +258,30 @@ SECTIONS _data = .; _sdata = .; + + __start_ro_after_init = .; + idmap_pg_dir = .; + . += PAGE_SIZE; + +#ifdef CONFIG_UNMAP_KERNEL_AT_EL0 + tramp_pg_dir = .; + . += PAGE_SIZE; +#endif + reserved_pg_dir = .; + . += PAGE_SIZE; + + swapper_pg_dir = .; + . += PAGE_SIZE; + + HYPERVISOR_DATA_SECTIONS + + .data.ro_after_init : { + *(.data..ro_after_init) + JUMP_TABLE_DATA + . = ALIGN(SEGMENT_ALIGN); + __end_ro_after_init = .; + } + RW_DATA(L1_CACHE_BYTES, PAGE_SIZE, THREAD_ALIGN) /* diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index 9828ad826837..e9b074ffc768 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -495,11 +495,17 @@ static void __init __map_memblock(pgd_t *pgdp, phys_addr_t start, void __init mark_linear_text_alias_ro(void) { /* - * Remove the write permissions from the linear alias of .text/.rodata + * Remove the write permissions from the linear alias of .text/.rodata/ro_after_init */ update_mapping_prot(__pa_symbol(_stext), (unsigned long)lm_alias(_stext), (unsigned long)__init_begin - (unsigned long)_stext, PAGE_KERNEL_RO); + + update_mapping_prot(__pa_symbol(__start_ro_after_init), + (unsigned long)lm_alias(__start_ro_after_init), + (unsigned long)__end_ro_after_init - + (unsigned long)__start_ro_after_init, + PAGE_KERNEL_RO); } static bool crash_mem_map __initdata; @@ -608,12 +614,10 @@ void mark_rodata_ro(void) { unsigned long section_size; - /* - * mark .rodata as read only. Use __init_begin rather than __end_rodata - * to cover NOTES and EXCEPTION_TABLE. - */ - section_size = (unsigned long)__init_begin - (unsigned long)__start_rodata; - update_mapping_prot(__pa_symbol(__start_rodata), (unsigned long)__start_rodata, + section_size = (unsigned long)__end_ro_after_init - + (unsigned long)__start_ro_after_init; + update_mapping_prot(__pa_symbol(__start_ro_after_init), + (unsigned long)__start_ro_after_init, section_size, PAGE_KERNEL_RO); debug_checkwx(); @@ -733,18 +737,19 @@ static void __init map_kernel(pgd_t *pgdp) text_prot = __pgprot_modify(text_prot, PTE_GP, PTE_GP); /* - * Only rodata will be remapped with different permissions later on, - * all other segments are allowed to use contiguous mappings. + * Only data will be partially remapped with different permissions + * later on, all other segments are allowed to use contiguous mappings. */ map_kernel_segment(pgdp, _stext, _etext, text_prot, &vmlinux_text, 0, VM_NO_GUARD); - map_kernel_segment(pgdp, __start_rodata, __inittext_begin, PAGE_KERNEL, - &vmlinux_rodata, NO_CONT_MAPPINGS, VM_NO_GUARD); + map_kernel_segment(pgdp, __start_rodata, __inittext_begin, PAGE_KERNEL_RO, + &vmlinux_rodata, 0, VM_NO_GUARD); map_kernel_segment(pgdp, __inittext_begin, __inittext_end, text_prot, &vmlinux_inittext, 0, VM_NO_GUARD); map_kernel_segment(pgdp, __initdata_begin, __initdata_end, PAGE_KERNEL, &vmlinux_initdata, 0, VM_NO_GUARD); - map_kernel_segment(pgdp, _data, _end, PAGE_KERNEL, &vmlinux_data, 0, 0); + map_kernel_segment(pgdp, _data, _end, PAGE_KERNEL, &vmlinux_data, + NO_CONT_MAPPINGS | NO_BLOCK_MAPPINGS, 0); if (!READ_ONCE(pgd_val(*pgd_offset_pgd(pgdp, FIXADDR_START)))) { /*
Currently, the ro_after_init sections sits right in the middle of the text/rodata/inittext segment, making it difficult to map any of those non-writable during early boot. So instead, move it to the start of .data, and update the init sequences so that the section is remapped read-only once startup completes. Note that this moves the entire HYP data section into .data as well - this likely needs to remain as a single block for now, but could perhaps split into a .rodata and .data..ro_after_init section later. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm64/kernel/vmlinux.lds.S | 42 ++++++++++++-------- arch/arm64/mm/mmu.c | 29 ++++++++------ 2 files changed, 42 insertions(+), 29 deletions(-)