Re: [PATCH v6 3/9] arm64, kfence: enable KFENCE for ARM64
On Fri, Oct 30, 2020 at 03:49:26AM +0100, Jann Horn wrote: > On Thu, Oct 29, 2020 at 2:17 PM Marco Elver wrote: > > @@ -312,6 +313,9 @@ static void __do_kernel_fault(unsigned long addr, > > unsigned int esr, > > "Ignoring spurious kernel translation fault at virtual address > > %016lx\n", addr)) > > return; > > > > + if (kfence_handle_page_fault(addr)) > > + return; > > As in the X86 case, we may want to ensure that this doesn't run for > permission faults, only for non-present pages. Maybe move this down > into the third branch of the "if" block below (neither permission > fault nor NULL deref)? I think that'd make sense. Those cases *should* be mutually exclusive, but it'd be more robust to do the KFENCE checks in that last block so that if something goes wrong wrong within KFENCE we can't get stuck in a loop failing to service an instruction abort or similar. Either that, or factor out an is_el1_translation_fault() and only do the KFENCE check and is_spurious_el1_translation_fault() check under that. Thanks, Mark.
Re: [PATCH v6 3/9] arm64, kfence: enable KFENCE for ARM64
On Fri, 30 Oct 2020 at 16:47, Mark Rutland wrote: > > On Thu, Oct 29, 2020 at 02:16:43PM +0100, Marco Elver wrote: > > Add architecture specific implementation details for KFENCE and enable > > KFENCE for the arm64 architecture. In particular, this implements the > > required interface in . > > > > KFENCE requires that attributes for pages from its memory pool can > > individually be set. Therefore, force the entire linear map to be mapped > > at page granularity. Doing so may result in extra memory allocated for > > page tables in case rodata=full is not set; however, currently > > CONFIG_RODATA_FULL_DEFAULT_ENABLED=y is the default, and the common case > > is therefore not affected by this change. > > > > Reviewed-by: Dmitry Vyukov > > Co-developed-by: Alexander Potapenko > > Signed-off-by: Alexander Potapenko > > Signed-off-by: Marco Elver > > --- > > v5: > > * Move generic page allocation code to core.c [suggested by Jann Horn]. > > * Remove comment about HAVE_ARCH_KFENCE_STATIC_POOL, since we no longer > > support static pools. > > * Force page granularity for the linear map [suggested by Mark Rutland]. > > --- > > arch/arm64/Kconfig | 1 + > > arch/arm64/include/asm/kfence.h | 19 +++ > > arch/arm64/mm/fault.c | 4 > > arch/arm64/mm/mmu.c | 7 ++- > > 4 files changed, 30 insertions(+), 1 deletion(-) > > create mode 100644 arch/arm64/include/asm/kfence.h > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > > index f858c352f72a..2f8b328b 100644 > > --- a/arch/arm64/Kconfig > > +++ b/arch/arm64/Kconfig > > @@ -135,6 +135,7 @@ config ARM64 > > select HAVE_ARCH_JUMP_LABEL_RELATIVE > > select HAVE_ARCH_KASAN if !(ARM64_16K_PAGES && ARM64_VA_BITS_48) > > select HAVE_ARCH_KASAN_SW_TAGS if HAVE_ARCH_KASAN > > + select HAVE_ARCH_KFENCE if (!ARM64_16K_PAGES && !ARM64_64K_PAGES) > > Why does this depend on the page size? > > If this is functional, but has a larger overhead on 16K or 64K, I'd > suggest removing the dependency, and just updating the Kconfig help text > to explain that. Good point, I don't think anything is requiring us to force 4K pages. Let's remove it. Thanks, -- Marco > Otherwise, this patch looks fine to me. > > Thanks, > Mark.
Re: [PATCH v6 3/9] arm64, kfence: enable KFENCE for ARM64
On Thu, Oct 29, 2020 at 02:16:43PM +0100, Marco Elver wrote: > Add architecture specific implementation details for KFENCE and enable > KFENCE for the arm64 architecture. In particular, this implements the > required interface in . > > KFENCE requires that attributes for pages from its memory pool can > individually be set. Therefore, force the entire linear map to be mapped > at page granularity. Doing so may result in extra memory allocated for > page tables in case rodata=full is not set; however, currently > CONFIG_RODATA_FULL_DEFAULT_ENABLED=y is the default, and the common case > is therefore not affected by this change. > > Reviewed-by: Dmitry Vyukov > Co-developed-by: Alexander Potapenko > Signed-off-by: Alexander Potapenko > Signed-off-by: Marco Elver > --- > v5: > * Move generic page allocation code to core.c [suggested by Jann Horn]. > * Remove comment about HAVE_ARCH_KFENCE_STATIC_POOL, since we no longer > support static pools. > * Force page granularity for the linear map [suggested by Mark Rutland]. > --- > arch/arm64/Kconfig | 1 + > arch/arm64/include/asm/kfence.h | 19 +++ > arch/arm64/mm/fault.c | 4 > arch/arm64/mm/mmu.c | 7 ++- > 4 files changed, 30 insertions(+), 1 deletion(-) > create mode 100644 arch/arm64/include/asm/kfence.h > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index f858c352f72a..2f8b328b 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -135,6 +135,7 @@ config ARM64 > select HAVE_ARCH_JUMP_LABEL_RELATIVE > select HAVE_ARCH_KASAN if !(ARM64_16K_PAGES && ARM64_VA_BITS_48) > select HAVE_ARCH_KASAN_SW_TAGS if HAVE_ARCH_KASAN > + select HAVE_ARCH_KFENCE if (!ARM64_16K_PAGES && !ARM64_64K_PAGES) Why does this depend on the page size? If this is functional, but has a larger overhead on 16K or 64K, I'd suggest removing the dependency, and just updating the Kconfig help text to explain that. Otherwise, this patch looks fine to me. Thanks, Mark.
Re: [PATCH v6 3/9] arm64, kfence: enable KFENCE for ARM64
On Thu, Oct 29, 2020 at 2:17 PM Marco Elver wrote:
> Add architecture specific implementation details for KFENCE and enable
> KFENCE for the arm64 architecture. In particular, this implements the
> required interface in .
>
> KFENCE requires that attributes for pages from its memory pool can
> individually be set. Therefore, force the entire linear map to be mapped
> at page granularity. Doing so may result in extra memory allocated for
> page tables in case rodata=full is not set; however, currently
> CONFIG_RODATA_FULL_DEFAULT_ENABLED=y is the default, and the common case
> is therefore not affected by this change.
[...]
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
[...]
> + select HAVE_ARCH_KFENCE if (!ARM64_16K_PAGES && !ARM64_64K_PAGES)
"if ARM64_4K_PAGES"?
[...]
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
[...]
> @@ -312,6 +313,9 @@ static void __do_kernel_fault(unsigned long addr,
> unsigned int esr,
> "Ignoring spurious kernel translation fault at virtual address
> %016lx\n", addr))
> return;
>
> + if (kfence_handle_page_fault(addr))
> + return;
As in the X86 case, we may want to ensure that this doesn't run for
permission faults, only for non-present pages. Maybe move this down
into the third branch of the "if" block below (neither permission
fault nor NULL deref)?
> +
> if (is_el1_permission_fault(addr, esr, regs)) {
> if (esr & ESR_ELx_WNR)
> msg = "write to read-only memory";
