Having memory that is writable and executable at the same time is a
security hazard, and so we tend to avoid those when we can. However,
at boot time, we keep .text mapped writable during the entire init
phase, and the init region itself is mapped rwx as well.
Let's improve the situation by:
- making the alternatives patching use the linear mapping
- splitting the init region into separate text and data regions
This removes all RWX mappings except the really early one created
in head.S (which we could perhaps fix in the future as well)
Changes since v1:
- add patch to move TLB maintenance into create_mapping_late() and remove it
from its callers (#2)
- use the true address not the linear alias when patching branch instructions,
spotted by Suzuki (#3)
- mark mark_linear_text_alias_ro() __init (#3)
- move the .rela section back into __initdata: as it turns out, leaving a hole
between the segments results in a peculiar situation where other unrelated
allocations end up right in the middle of the kernel Image, which is
probably a bad idea (#5). See below for an example.
- add acks
Ard Biesheuvel (5):
arm: kvm: move kvm_vgic_global_state out of .text section
arm64: mmu: move TLB maintenance from callers to create_mapping_late()
arm64: alternatives: apply boot time fixups via the linear mapping
arm64: mmu: map .text as read-only from the outset
arm64: mmu: apply strict permissions to .init.text and .init.data
arch/arm64/include/asm/mmu.h | 1 +
arch/arm64/include/asm/sections.h | 3 +-
arch/arm64/kernel/alternative.c | 2 +-
arch/arm64/kernel/smp.c | 1 +
arch/arm64/kernel/vmlinux.lds.S | 25 +++++++----
arch/arm64/mm/mmu.c | 45 +++++++++++++-------
virt/kvm/arm/vgic/vgic.c | 4 +-
7 files changed, 53 insertions(+), 28 deletions(-)
--
2.7.4
The various kernel segments are vmapped from paging_init() [after inlining]
0xffffff8008080000-0xffffff80088b0000 8585216 paging_init+0x84/0x584 phys=40080000 vmap
0xffffff80088b0000-0xffffff8008cb0000 4194304 paging_init+0xa4/0x584 phys=408b0000 vmap
0xffffff8008cb0000-0xffffff8008d27000 487424 paging_init+0xc4/0x584 phys=40cb0000 vmap
0xffffff8008d27000-0xffffff8008da3000 507904 paging_init+0xe8/0x584 phys=40d27000 vmap
0xffffff8008dd1000-0xffffff8008dd3000 8192 devm_ioremap_nocache+0x54/0xa8 phys=a003000 ioremap
0xffffff8008dd3000-0xffffff8008dd5000 8192 devm_ioremap_nocache+0x54/0xa8 phys=a003000 ioremap
0xffffff8008dde000-0xffffff8008de0000 8192 pl031_probe+0x80/0x1e8 phys=9010000 ioremap
0xffffff8008e4c000-0xffffff8008e50000 16384 n_tty_open+0x1c/0xd0 pages=3 vmalloc
0xffffff8008e54000-0xffffff8008e58000 16384 n_tty_open+0x1c/0xd0 pages=3 vmalloc
0xffffff8008e80000-0xffffff8008e84000 16384 n_tty_open+0x1c/0xd0 pages=3 vmalloc
0xffffff8008e84000-0xffffff8008e88000 16384 n_tty_open+0x1c/0xd0 pages=3 vmalloc
0xffffff8008ea0000-0xffffff8008ea2000 8192 bpf_prog_alloc+0x3c/0xb8 pages=1 vmalloc
0xffffff8008ef2000-0xffffff8008ef6000 16384 n_tty_open+0x1c/0xd0 pages=3 vmalloc
0xffffff8008ef6000-0xffffff8008efa000 16384 n_tty_open+0x1c/0xd0 pages=3 vmalloc
0xffffff8009010000-0xffffff800914b000 1290240 paging_init+0x10c/0x584 phys=41010000 vmap
The kvm_vgic_global_state struct contains a static key which is
written to by jump_label_init() at boot time. So in preparation of
making .text regions truly (well, almost truly) read-only, mark
kvm_vgic_global_state __ro_after_init so it moves to the .rodata
section instead.
Acked-by: Marc Zyngier <redacted>
Reviewed-by: Laura Abbott <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
---
virt/kvm/arm/vgic/vgic.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
In preparation of changing the way we invoke create_mapping_late() (which
is currently invoked twice from the same function), move the TLB flushing
it performs from the caller into create_mapping_late() itself, and change
it to a TLB maintenance by VA rather than a full flush, which is more
appropriate here.
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/mm/mmu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -356,6 +356,9 @@ static void create_mapping_late(phys_addr_t phys, unsigned long virt,__create_pgd_mapping(init_mm.pgd,phys,virt,size,prot,NULL,debug_pagealloc_enabled());++/* flush the TLBs after updating live kernel mappings */+flush_tlb_kernel_range(virt,virt+size);}staticvoid__init__map_memblock(pgd_t*pgd,phys_addr_tstart,phys_addr_tend)
@@ -438,9 +441,6 @@ void mark_rodata_ro(void)create_mapping_late(__pa_symbol(__start_rodata),(unsignedlong)__start_rodata,section_size,PAGE_KERNEL_RO);-/* flush the TLBs after updating live kernel mappings */-flush_tlb_all();-debug_checkwx();}
One important rule of thumb when desiging a secure software system is
that memory should never be writable and executable at the same time.
We mostly adhere to this rule in the kernel, except at boot time, when
regions may be mapped RWX until after we are done applying alternatives
or making other one-off changes.
For the alternative patching, we can improve the situation by applying
the fixups via the linear mapping, which is never mapped with executable
permissions. So map the linear alias of .text with RW- permissions
initially, and remove the write permissions as soon as alternative
patching has completed.
Reviewed-by: Laura Abbott <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/include/asm/mmu.h | 1 +
arch/arm64/kernel/alternative.c | 2 +-
arch/arm64/kernel/smp.c | 1 +
arch/arm64/mm/mmu.c | 22 +++++++++++++++-----
4 files changed, 20 insertions(+), 6 deletions(-)
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 <redacted>
Reviewed-by: Kees Cook <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/mm/mmu.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
To avoid having mappings that are writable and executable at the same
time, split the init region into a .init.text region that is mapped
read-only, and a .init.data region that is mapped non-executable.
This is possible now that the alternative patching occurs via the linear
mapping, and the linear alias of the init region is always mapped writable
(but never executable).
Since the alternatives descriptions themselves are read-only data, move
those into the .init.text region.
Reviewed-by: Laura Abbott <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/include/asm/sections.h | 3 ++-
arch/arm64/kernel/vmlinux.lds.S | 25 +++++++++++++-------
arch/arm64/mm/mmu.c | 12 ++++++----
3 files changed, 26 insertions(+), 14 deletions(-)
On Sat, Feb 11, 2017 at 08:23:03PM +0000, Ard Biesheuvel wrote:
In preparation of changing the way we invoke create_mapping_late() (which
is currently invoked twice from the same function), move the TLB flushing
it performs from the caller into create_mapping_late() itself, and change
it to a TLB maintenance by VA rather than a full flush, which is more
appropriate here.
It's not immediately clear what's meant by "changing the way we invoke
create_mapping_late()" here.
It's probably worth explicitly mentioning that we need to add another
caller of create_mapping_late(), and this saves us adding (overly
strong) TLB maintenance to all callers.
@@ -356,6 +356,9 @@ static void create_mapping_late(phys_addr_t phys, unsigned long virt,__create_pgd_mapping(init_mm.pgd,phys,virt,size,prot,NULL,debug_pagealloc_enabled());++/* flush the TLBs after updating live kernel mappings */+flush_tlb_kernel_range(virt,virt+size);}
It feels a little odd to have the maintenance here given we still call
this *create*_mapping_late.
Given the only users of this are changing permissions, perhaps we should
rename this to change_mapping_prot(), or something like that?
Otherwise, this looks fine to me, and boots fine. Either way:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Thanks,
Mark.
On Sat, Feb 11, 2017 at 08:23:04PM +0000, Ard Biesheuvel wrote:
One important rule of thumb when desiging a secure software system is
that memory should never be writable and executable at the same time.
We mostly adhere to this rule in the kernel, except at boot time, when
regions may be mapped RWX until after we are done applying alternatives
or making other one-off changes.
For the alternative patching, we can improve the situation by applying
the fixups via the linear mapping, which is never mapped with executable
permissions. So map the linear alias of .text with RW- permissions
initially, and remove the write permissions as soon as alternative
patching has completed.
Reviewed-by: Laura Abbott <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Mark.
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 <redacted>
Reviewed-by: Kees Cook <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
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.
On Sat, Feb 11, 2017 at 08:23:06PM +0000, Ard Biesheuvel wrote:
To avoid having mappings that are writable and executable at the same
time, split the init region into a .init.text region that is mapped
read-only, and a .init.data region that is mapped non-executable.
This is possible now that the alternative patching occurs via the linear
mapping, and the linear alias of the init region is always mapped writable
(but never executable).
Since the alternatives descriptions themselves are read-only data, move
those into the .init.text region.
Reviewed-by: Laura Abbott <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
This generally looks good.
As with my comment on patch 4, we might want to allow .init.text to be
mapped writeable for the sake of external debuggers.
Thanks,
Mark.
On 14 Feb 2017, at 15:57, Mark Rutland [off-list ref] wrote:
quoted
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 <redacted>
Reviewed-by: Kees Cook <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
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)
{
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.
On Tue, Feb 14, 2017 at 04:15:11PM +0000, Ard Biesheuvel wrote:
quoted
On 14 Feb 2017, at 15:57, Mark Rutland [off-list ref] wrote:
quoted
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 <redacted>
Reviewed-by: Kees Cook <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
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.
quoted
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 [off-list ref] wrote:
quoted
On Tue, Feb 14, 2017 at 04:15:11PM +0000, Ard Biesheuvel wrote:
quoted
quoted
On 14 Feb 2017, at 15:57, Mark Rutland [off-list ref] 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 <redacted>
Reviewed-by: Kees Cook <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
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
quoted
quoted
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.
quoted
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:
quoted
On 14 Feb 2017, at 17:40, Mark Rutland [off-list ref] wrote:
quoted
On Tue, Feb 14, 2017 at 04:15:11PM +0000, Ard Biesheuvel wrote:
quoted
quoted
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)