| Message ID | 1486844586-26135-5-git-send-email-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Feb 11, 2017 at 08:23:05PM +0000, Ard Biesheuvel wrote: > Now that alternatives patching code no longer relies on the primary > mapping of .text being writable, we can remove the code that removes > the writable permissions post-init time, and map it read-only from > the outset. > > Reviewed-by: Laura Abbott <labbott@redhat.com> > Reviewed-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> This generally looks good. One effect of this is that even with rodata=off, external debuggers can't install SW breakpoints via the executable mapping. We might want to allow that to be overridden. e.g. make rodata= an early param, and switch the permissions based on that in map_kernel(), e.g. have: pgprot_t text_prot = rodata_enabled ? PAGE_KERNEL_ROX : PAGE_KERNEL_EXEC); ... and use that for .text and .init.text by default. Thanks, Mark. > --- > arch/arm64/mm/mmu.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 7ed981c7f4c0..e97f1ce967ec 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -442,9 +442,6 @@ void mark_rodata_ro(void) > { > unsigned long section_size; > > - section_size = (unsigned long)_etext - (unsigned long)_text; > - create_mapping_late(__pa_symbol(_text), (unsigned long)_text, > - section_size, PAGE_KERNEL_ROX); > /* > * mark .rodata as read only. Use __init_begin rather than __end_rodata > * to cover NOTES and EXCEPTION_TABLE. > @@ -484,7 +481,7 @@ static void __init map_kernel(pgd_t *pgd) > { > static struct vm_struct vmlinux_text, vmlinux_rodata, vmlinux_init, vmlinux_data; > > - map_kernel_segment(pgd, _text, _etext, PAGE_KERNEL_EXEC, &vmlinux_text); > + map_kernel_segment(pgd, _text, _etext, PAGE_KERNEL_ROX, &vmlinux_text); > map_kernel_segment(pgd, __start_rodata, __init_begin, PAGE_KERNEL, &vmlinux_rodata); > map_kernel_segment(pgd, __init_begin, __init_end, PAGE_KERNEL_EXEC, > &vmlinux_init); > -- > 2.7.4 >
> On 14 Feb 2017, at 15:57, Mark Rutland <mark.rutland@arm.com> wrote: > >> On Sat, Feb 11, 2017 at 08:23:05PM +0000, Ard Biesheuvel wrote: >> Now that alternatives patching code no longer relies on the primary >> mapping of .text being writable, we can remove the code that removes >> the writable permissions post-init time, and map it read-only from >> the outset. >> >> Reviewed-by: Laura Abbott <labbott@redhat.com> >> Reviewed-by: Kees Cook <keescook@chromium.org> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > This generally looks good. > > One effect of this is that even with rodata=off, external debuggers > can't install SW breakpoints via the executable mapping. > Interesting. For the sake of my education, could you elaborate on how that works under the hood? > We might want to allow that to be overridden. e.g. make rodata= an > early param, and switch the permissions based on that in map_kernel(), > e.g. have: > > pgprot_t text_prot = rodata_enabled ? PAGE_KERNEL_ROX > : PAGE_KERNEL_EXEC); > > ... and use that for .text and .init.text by default. > > Is there any way we could restrict this privilege to external debuggers? Having trivial 'off' switches for security features makes me feel uneasy (although this is orthogonal to this patch) >> --- >> arch/arm64/mm/mmu.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >> index 7ed981c7f4c0..e97f1ce967ec 100644 >> --- a/arch/arm64/mm/mmu.c >> +++ b/arch/arm64/mm/mmu.c >> @@ -442,9 +442,6 @@ void mark_rodata_ro(void) >> { >> unsigned long section_size; >> >> - section_size = (unsigned long)_etext - (unsigned long)_text; >> - create_mapping_late(__pa_symbol(_text), (unsigned long)_text, >> - section_size, PAGE_KERNEL_ROX); >> /* >> * mark .rodata as read only. Use __init_begin rather than __end_rodata >> * to cover NOTES and EXCEPTION_TABLE. >> @@ -484,7 +481,7 @@ static void __init map_kernel(pgd_t *pgd) >> { >> static struct vm_struct vmlinux_text, vmlinux_rodata, vmlinux_init, vmlinux_data; >> >> - map_kernel_segment(pgd, _text, _etext, PAGE_KERNEL_EXEC, &vmlinux_text); >> + map_kernel_segment(pgd, _text, _etext, PAGE_KERNEL_ROX, &vmlinux_text); >> map_kernel_segment(pgd, __start_rodata, __init_begin, PAGE_KERNEL, &vmlinux_rodata); >> map_kernel_segment(pgd, __init_begin, __init_end, PAGE_KERNEL_EXEC, >> &vmlinux_init); >> -- >> 2.7.4 >>
On Tue, Feb 14, 2017 at 04:15:11PM +0000, Ard Biesheuvel wrote: > > > On 14 Feb 2017, at 15:57, Mark Rutland <mark.rutland@arm.com> wrote: > > > >> On Sat, Feb 11, 2017 at 08:23:05PM +0000, Ard Biesheuvel wrote: > >> Now that alternatives patching code no longer relies on the primary > >> mapping of .text being writable, we can remove the code that removes > >> the writable permissions post-init time, and map it read-only from > >> the outset. > >> > >> Reviewed-by: Laura Abbott <labbott@redhat.com> > >> Reviewed-by: Kees Cook <keescook@chromium.org> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > This generally looks good. > > > > One effect of this is that even with rodata=off, external debuggers > > can't install SW breakpoints via the executable mapping. > > Interesting. For the sake of my education, could you elaborate on how > that works under the hood? There are details in ARM DDI 0487A.k_iss10775, Chapter H1, "About External Debug", page H1-4839 onwards. Otherwise, executive summary below. An external debugger can place a CPU into debug state. This is orthogonal to execution state and exception level, which are unchanged. While in this state, the CPU (only) executes instructions fed to it by the debugger through a special register. To install a SW breakpoint, the debugger makes the CPU enter debug state, then issues regular stores, barriers, and cache maintenance. These operate in the current execution state at the current EL, using the current translation regime. The external debugger can also trap exceptions (e.g. those caused by the SW breakpoint). The CPU enters debug state when these are trapped. > > We might want to allow that to be overridden. e.g. make rodata= an > > early param, and switch the permissions based on that in map_kernel(), > > e.g. have: > > > > pgprot_t text_prot = rodata_enabled ? PAGE_KERNEL_ROX > > : PAGE_KERNEL_EXEC); > > > > ... and use that for .text and .init.text by default. > > > > > > Is there any way we could restrict this privilege to external > debuggers? My understanding is that we cannot. > Having trivial 'off' switches for security features makes me feel > uneasy (although this is orthogonal to this patch) From my PoV, external debuggers are the sole reason to allow rodata=off for arm64, and we already allow rodata=off. Thanks, Mark.
> On 14 Feb 2017, at 17:40, Mark Rutland <mark.rutland@arm.com> wrote: > >> On Tue, Feb 14, 2017 at 04:15:11PM +0000, Ard Biesheuvel wrote: >> >>>> On 14 Feb 2017, at 15:57, Mark Rutland <mark.rutland@arm.com> wrote: >>>> >>>> On Sat, Feb 11, 2017 at 08:23:05PM +0000, Ard Biesheuvel wrote: >>>> Now that alternatives patching code no longer relies on the primary >>>> mapping of .text being writable, we can remove the code that removes >>>> the writable permissions post-init time, and map it read-only from >>>> the outset. >>>> >>>> Reviewed-by: Laura Abbott <labbott@redhat.com> >>>> Reviewed-by: Kees Cook <keescook@chromium.org> >>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> >>> This generally looks good. >>> >>> One effect of this is that even with rodata=off, external debuggers >>> can't install SW breakpoints via the executable mapping. >> >> Interesting. For the sake of my education, could you elaborate on how >> that works under the hood? > > There are details in ARM DDI 0487A.k_iss10775, Chapter H1, "About > External Debug", page H1-4839 onwards. Otherwise, executive summary > below. > > An external debugger can place a CPU into debug state. This is > orthogonal to execution state and exception level, which are unchanged. > While in this state, the CPU (only) executes instructions fed to it by > the debugger through a special register. > > To install a SW breakpoint, the debugger makes the CPU enter debug > state, then issues regular stores, barriers, and cache maintenance. > These operate in the current execution state at the current EL, using > the current translation regime. > > The external debugger can also trap exceptions (e.g. those caused by the > SW breakpoint). The CPU enters debug state when these are trapped. > OK, thanks for the explanation >>> We might want to allow that to be overridden. e.g. make rodata= an >>> early param, and switch the permissions based on that in map_kernel(), >>> e.g. have: >>> >>> pgprot_t text_prot = rodata_enabled ? PAGE_KERNEL_ROX >>> : PAGE_KERNEL_EXEC); >>> >>> ... and use that for .text and .init.text by default. >>> >>> >> >> Is there any way we could restrict this privilege to external >> debuggers? > > My understanding is that we cannot. > >> Having trivial 'off' switches for security features makes me feel >> uneasy (although this is orthogonal to this patch) > > From my PoV, external debuggers are the sole reason to allow rodata=off > for arm64, and we already allow rodata=off. > > Indeed. If that is how it works currently, we shouldn't interfere with it. If we ever get anywhere with the lockdown patches, we should blacklist this parameter (or rather, not whitelist it, since blacklisting kernel params to enforce security is infeasible imo)
On Tue, Feb 14, 2017 at 05:49:19PM +0000, Ard Biesheuvel wrote: > > > On 14 Feb 2017, at 17:40, Mark Rutland <mark.rutland@arm.com> wrote: > > > >> On Tue, Feb 14, 2017 at 04:15:11PM +0000, Ard Biesheuvel wrote: > >> Having trivial 'off' switches for security features makes me feel > >> uneasy (although this is orthogonal to this patch) > > > > From my PoV, external debuggers are the sole reason to allow rodata=off > > for arm64, and we already allow rodata=off. > > > > > > Indeed. If that is how it works currently, we shouldn't interfere with > it. If we ever get anywhere with the lockdown patches, we should > blacklist this parameter (or rather, not whitelist it, since > blacklisting kernel params to enforce security is infeasible imo) Agreed on all counts! Mark.
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index 7ed981c7f4c0..e97f1ce967ec 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -442,9 +442,6 @@ void mark_rodata_ro(void) { unsigned long section_size; - section_size = (unsigned long)_etext - (unsigned long)_text; - create_mapping_late(__pa_symbol(_text), (unsigned long)_text, - section_size, PAGE_KERNEL_ROX); /* * mark .rodata as read only. Use __init_begin rather than __end_rodata * to cover NOTES and EXCEPTION_TABLE. @@ -484,7 +481,7 @@ static void __init map_kernel(pgd_t *pgd) { static struct vm_struct vmlinux_text, vmlinux_rodata, vmlinux_init, vmlinux_data; - map_kernel_segment(pgd, _text, _etext, PAGE_KERNEL_EXEC, &vmlinux_text); + map_kernel_segment(pgd, _text, _etext, PAGE_KERNEL_ROX, &vmlinux_text); map_kernel_segment(pgd, __start_rodata, __init_begin, PAGE_KERNEL, &vmlinux_rodata); map_kernel_segment(pgd, __init_begin, __init_end, PAGE_KERNEL_EXEC, &vmlinux_init);