lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 13 Jun 2022 09:51:11 -0700 From: Kees Cook <keescook@...omium.org> To: Ard Biesheuvel <ardb@...nel.org> Cc: linux-arm-kernel@...ts.infradead.org, linux-hardening@...r.kernel.org, Marc Zyngier <maz@...nel.org>, Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>, Catalin Marinas <catalin.marinas@....com>, Mark Brown <broonie@...nel.org>, Anshuman Khandual <anshuman.khandual@....com> Subject: Re: [PATCH v4 25/26] arm64: mm: add support for WXN memory translation attribute On Mon, Jun 13, 2022 at 04:45:49PM +0200, Ard Biesheuvel wrote: > The AArch64 virtual memory system supports a global WXN control, which > can be enabled to make all writable mappings implicitly no-exec. This is > a useful hardening feature, as it prevents mistakes in managing page > table permissions from being exploited to attack the system. > > When enabled at EL1, the restrictions apply to both EL1 and EL0. EL1 is > completely under our control, and has been cleaned up to allow WXN to be > enabled from boot onwards. EL0 is not under our control, but given that > widely deployed security features such as selinux or PaX already limit > the ability of user space to create mappings that are writable and > executable at the same time, the impact of enabling this for EL0 is > expected to be limited. (For this reason, common user space libraries > that have a legitimate need for manipulating executable code already > carry fallbacks such as [0].) > > If enabled at compile time, the feature can still be disabled at boot if > needed, by passing arm64.nowxn on the kernel command line. > > [0] https://github.com/libffi/libffi/blob/master/src/closures.c#L440 > > Signed-off-by: Ard Biesheuvel <ardb@...nel.org> > --- > arch/arm64/Kconfig | 11 ++++++ > arch/arm64/include/asm/cpufeature.h | 8 +++++ > arch/arm64/include/asm/mman.h | 36 ++++++++++++++++++++ > arch/arm64/include/asm/mmu_context.h | 30 +++++++++++++++- > arch/arm64/kernel/head.S | 28 ++++++++++++++- > arch/arm64/kernel/idreg-override.c | 16 +++++++++ > arch/arm64/mm/proc.S | 6 ++++ > 7 files changed, 133 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 1652a9800ebe..d262d5ab4316 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -1422,6 +1422,17 @@ config RODATA_FULL_DEFAULT_ENABLED > This requires the linear region to be mapped down to pages, > which may adversely affect performance in some cases. > > +config ARM64_WXN > + bool "Enable WXN attribute so all writable mappings are non-exec" > + help > + Set the WXN bit in the SCTLR system register so that all writable > + mappings are treated as if the PXN/UXN bit is set as well. > + If this is set to Y, it can still be disabled at runtime by > + passing 'arm64.nowxn' on the kernel command line. > + > + This should only be set if no software needs to be supported that > + relies on being able to execute from writable mappings. Should this instead just be a "default value of arm64.xwn" config? It seems like it should be possible to just drop all the #ifdefs below, as XWN is arguably the default state we would want systems to move to. > + > config ARM64_SW_TTBR0_PAN > bool "Emulate Privileged Access Never using TTBR0_EL1 switching" > help > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index 14a8f3d93add..fc364c4d31e2 100644 > --- a/arch/arm64/include/asm/cpufeature.h > +++ b/arch/arm64/include/asm/cpufeature.h > @@ -911,10 +911,18 @@ extern struct arm64_ftr_override id_aa64mmfr1_override; > extern struct arm64_ftr_override id_aa64pfr1_override; > extern struct arm64_ftr_override id_aa64isar1_override; > extern struct arm64_ftr_override id_aa64isar2_override; > +extern struct arm64_ftr_override sctlr_override; > > u32 get_kvm_ipa_limit(void); > void dump_cpu_features(void); > > +static inline bool arm64_wxn_enabled(void) > +{ > + if (!IS_ENABLED(CONFIG_ARM64_WXN)) > + return false; > + return (sctlr_override.val & sctlr_override.mask & 0xf) == 0; > +} > + > #endif /* __ASSEMBLY__ */ > > #endif > diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h > index 5966ee4a6154..6d4940342ba7 100644 > --- a/arch/arm64/include/asm/mman.h > +++ b/arch/arm64/include/asm/mman.h > @@ -35,11 +35,40 @@ static inline unsigned long arch_calc_vm_flag_bits(unsigned long flags) > } > #define arch_calc_vm_flag_bits(flags) arch_calc_vm_flag_bits(flags) > > +static inline bool arm64_check_wx_prot(unsigned long prot, > + struct task_struct *tsk) > +{ > + /* > + * When we are running with SCTLR_ELx.WXN==1, writable mappings are > + * implicitly non-executable. This means we should reject such mappings > + * when user space attempts to create them using mmap() or mprotect(). If this series is respun, perhaps add to this comment a little to indicate that this is basically a hint to userspace, and not an attempt to actually provide a general W+X mapping protection: * Note that this is effectively just a hint (for things like * libffi noted below), as solving this for all mapping combinations * is a larger endeavor. (e.g. userspace setting an executable mapping * writable, changing it, and then making it read-only again.) > + */ > + if (arm64_wxn_enabled() && > + ((prot & (PROT_WRITE | PROT_EXEC)) == (PROT_WRITE | PROT_EXEC))) { > + /* > + * User space libraries such as libffi carry elaborate > + * heuristics to decide whether it is worth it to even attempt > + * to create writable executable mappings, as PaX or selinux > + * enabled systems will outright reject it. They will usually > + * fall back to something else (e.g., two separate shared > + * mmap()s of a temporary file) on failure. > + */ > + pr_info_ratelimited( > + "process %s (%d) attempted to create PROT_WRITE+PROT_EXEC mapping\n", > + tsk->comm, tsk->pid); > + return false; > + } > + return true; > +} But regardless, with or without the changes above: Reviewed-by: Kees Cook <keescook@...omium.org> -- Kees Cook
Powered by blists - more mailing lists