| Message ID | 1455293599-6974-1-git-send-email-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 12, 2016 at 10:13:19AM -0600, Jeremy Linton wrote: > Currently the .rodata section is actually still executable when DEBUG_RODATA > is enabled. This changes that so the .rodata is actually read only, no execute. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > arch/arm64/kernel/vmlinux.lds.S | 5 +++-- > arch/arm64/mm/mmu.c | 14 +++++++++++--- > 2 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S > index ab4e436..2e2c053 100644 > --- a/arch/arm64/kernel/vmlinux.lds.S > +++ b/arch/arm64/kernel/vmlinux.lds.S > @@ -114,8 +114,9 @@ SECTIONS > *(.got) /* Global offset table */ > } > > - RO_DATA(PAGE_SIZE) > - EXCEPTION_TABLE(8) > + ALIGN_DEBUG_RO_MIN(0) > + RO_DATA(PAGE_SIZE) /* everything from this point to */ > + EXCEPTION_TABLE(8) /* _etext will be marked RO NX */ > NOTES > > ALIGN_DEBUG_RO_MIN(PAGE_SIZE) > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index ab69a99..a3f4112 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -453,10 +453,18 @@ static void __init map_mem(pgd_t *pgd) > #ifdef CONFIG_DEBUG_RODATA > void mark_rodata_ro(void) > { > - create_mapping_late(__pa(_stext), (unsigned long)_stext, > - (unsigned long)_etext - (unsigned long)_stext, > - PAGE_KERNEL_ROX); > + unsigned long section_size; > > + section_size = (unsigned long)__start_rodata - (unsigned long)_stext; > + create_mapping_late(__pa(_stext), (unsigned long)_stext, > + section_size, PAGE_KERNEL_ROX); > + /* > + * mark .rodata as read only. Use _etext rather than __end_rodata to > + * cover NOTES and EXCEPTION_TABLE. > + */ > + section_size = (unsigned long)_etext - (unsigned long)__start_rodata; > + create_mapping_late(__pa(__start_rodata), (unsigned long)__start_rodata, > + section_size, PAGE_KERNEL_RO); > } As you pointed out in the other thread, we'll also need to update map_kernel to use equivalent chunks for .text and .rodata. I think we can probably make .rodata RO from the outset in map_kernel, too. Could you please update mem_init to log .text and .rodata separately? It looks like some core code makes assumptions about _etext, so I guess that has to cover .rodata regardless. Otherwise, looks good! Thanks, Mark.
On Fri, Feb 12, 2016 at 10:25 AM, Mark Rutland <mark.rutland@arm.com> wrote: > On Fri, Feb 12, 2016 at 10:13:19AM -0600, Jeremy Linton wrote: >> Currently the .rodata section is actually still executable when DEBUG_RODATA >> is enabled. This changes that so the .rodata is actually read only, no execute. >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> Yikes, good catch. Is anyone running the lkdtm tests that check these things? Reviewed-by: Kees Cook <keescook@chromium.org> >> --- >> arch/arm64/kernel/vmlinux.lds.S | 5 +++-- >> arch/arm64/mm/mmu.c | 14 +++++++++++--- >> 2 files changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S >> index ab4e436..2e2c053 100644 >> --- a/arch/arm64/kernel/vmlinux.lds.S >> +++ b/arch/arm64/kernel/vmlinux.lds.S >> @@ -114,8 +114,9 @@ SECTIONS >> *(.got) /* Global offset table */ >> } >> >> - RO_DATA(PAGE_SIZE) >> - EXCEPTION_TABLE(8) >> + ALIGN_DEBUG_RO_MIN(0) >> + RO_DATA(PAGE_SIZE) /* everything from this point to */ >> + EXCEPTION_TABLE(8) /* _etext will be marked RO NX */ >> NOTES >> >> ALIGN_DEBUG_RO_MIN(PAGE_SIZE) >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >> index ab69a99..a3f4112 100644 >> --- a/arch/arm64/mm/mmu.c >> +++ b/arch/arm64/mm/mmu.c >> @@ -453,10 +453,18 @@ static void __init map_mem(pgd_t *pgd) >> #ifdef CONFIG_DEBUG_RODATA >> void mark_rodata_ro(void) >> { >> - create_mapping_late(__pa(_stext), (unsigned long)_stext, >> - (unsigned long)_etext - (unsigned long)_stext, >> - PAGE_KERNEL_ROX); >> + unsigned long section_size; >> >> + section_size = (unsigned long)__start_rodata - (unsigned long)_stext; >> + create_mapping_late(__pa(_stext), (unsigned long)_stext, >> + section_size, PAGE_KERNEL_ROX); >> + /* >> + * mark .rodata as read only. Use _etext rather than __end_rodata to >> + * cover NOTES and EXCEPTION_TABLE. >> + */ >> + section_size = (unsigned long)_etext - (unsigned long)__start_rodata; >> + create_mapping_late(__pa(__start_rodata), (unsigned long)__start_rodata, >> + section_size, PAGE_KERNEL_RO); >> } > > As you pointed out in the other thread, we'll also need to update > map_kernel to use equivalent chunks for .text and .rodata. > > I think we can probably make .rodata RO from the outset in map_kernel, > too. There's a benefit to marking it late in that .rodata can live in the same (upcoming) section as .data..ro_after_init, which is marked ro after mark_rodata_ro() to help constify more things. > Could you please update mem_init to log .text and .rodata separately? > > It looks like some core code makes assumptions about _etext, so I guess > that has to cover .rodata regardless. > > Otherwise, looks good! > > Thanks, > Mark. -Kees
On 2/16/16 10:10 AM, Kees Cook wrote: > On Fri, Feb 12, 2016 at 10:25 AM, Mark Rutland <mark.rutland@arm.com> wrote: >> On Fri, Feb 12, 2016 at 10:13:19AM -0600, Jeremy Linton wrote: >>> Currently the .rodata section is actually still executable when DEBUG_RODATA >>> is enabled. This changes that so the .rodata is actually read only, no execute. >>> >>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > > Yikes, good catch. Is anyone running the lkdtm tests that check these things? > I don't think the current lkdtm test would have caught this since the exec test is using rw data and not ro data. That test could be expanded though to include a rodata buffer as well. Thanks, Laura
On Tue, Feb 16, 2016 at 10:48 AM, Laura Abbott <laura@labbott.name> wrote: > On 2/16/16 10:10 AM, Kees Cook wrote: >> >> On Fri, Feb 12, 2016 at 10:25 AM, Mark Rutland <mark.rutland@arm.com> >> wrote: >>> >>> On Fri, Feb 12, 2016 at 10:13:19AM -0600, Jeremy Linton wrote: >>>> >>>> Currently the .rodata section is actually still executable when >>>> DEBUG_RODATA >>>> is enabled. This changes that so the .rodata is actually read only, no >>>> execute. >>>> >>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> >> >> Yikes, good catch. Is anyone running the lkdtm tests that check these >> things? >> > > I don't think the current lkdtm test would have caught this since the exec > test is using rw data and not ro data. That test could be expanded though > to include a rodata buffer as well. Oh, yeah, excellent point. I'll send a patch. -Kees
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S index ab4e436..2e2c053 100644 --- a/arch/arm64/kernel/vmlinux.lds.S +++ b/arch/arm64/kernel/vmlinux.lds.S @@ -114,8 +114,9 @@ SECTIONS *(.got) /* Global offset table */ } - RO_DATA(PAGE_SIZE) - EXCEPTION_TABLE(8) + ALIGN_DEBUG_RO_MIN(0) + RO_DATA(PAGE_SIZE) /* everything from this point to */ + EXCEPTION_TABLE(8) /* _etext will be marked RO NX */ NOTES ALIGN_DEBUG_RO_MIN(PAGE_SIZE) diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index ab69a99..a3f4112 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -453,10 +453,18 @@ static void __init map_mem(pgd_t *pgd) #ifdef CONFIG_DEBUG_RODATA void mark_rodata_ro(void) { - create_mapping_late(__pa(_stext), (unsigned long)_stext, - (unsigned long)_etext - (unsigned long)_stext, - PAGE_KERNEL_ROX); + unsigned long section_size; + section_size = (unsigned long)__start_rodata - (unsigned long)_stext; + create_mapping_late(__pa(_stext), (unsigned long)_stext, + section_size, PAGE_KERNEL_ROX); + /* + * mark .rodata as read only. Use _etext rather than __end_rodata to + * cover NOTES and EXCEPTION_TABLE. + */ + section_size = (unsigned long)_etext - (unsigned long)__start_rodata; + create_mapping_late(__pa(__start_rodata), (unsigned long)__start_rodata, + section_size, PAGE_KERNEL_RO); } #endif
Currently the .rodata section is actually still executable when DEBUG_RODATA is enabled. This changes that so the .rodata is actually read only, no execute. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- arch/arm64/kernel/vmlinux.lds.S | 5 +++-- arch/arm64/mm/mmu.c | 14 +++++++++++--- 2 files changed, 14 insertions(+), 5 deletions(-)