| 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <202206130940.2CFAF97@keescook>
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