linux-kernel - Re: [PATCHv2 4/4] arm64: dump: Add checking for writable and exectuable pages

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Oct 2016 13:47:21 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Laura Abbott <labbott@...hat.com>
Cc:     AKASHI Takahiro <takahiro.akashi@...aro.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Will Deacon <will.deacon@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Kees Cook <keescook@...omium.org>,
        kernel-hardening@...ts.openwall.com
Subject: Re: [PATCHv2 4/4] arm64: dump: Add checking for writable and
 exectuable pages

On Wed, Oct 12, 2016 at 03:32:02PM -0700, Laura Abbott wrote:
> +config DEBUG_WX
> +	bool "Warn on W+X mappings at boot"
> +	select ARM64_PTDUMP_CORE
> +	---help---
> +	  Generate a warning if any W+X mappings are found at boot.
> +
> +	  This is useful for discovering cases where the kernel is leaving
> +	  W+X mappings after applying NX, as such mappings are a security risk.
> +	  This check also includes UXN, which should be set on all kernel
> +	  mappings.
> +
> +	  Look for a message in dmesg output like this:
> +
> +	    arm64/mm: Checked W+X mappings: passed, no W+X pages found.
> +
> +	  or like this, if the check failed:
> +
> +	    arm64/mm: Checked W+X mappings: FAILED, <N> W+X pages found.
> +
> +	  Note that even if the check fails, your kernel is possibly
> +	  still fine, as W+X mappings are not a security hole in
> +	  themselves, what they do is that they make the exploitation
> +	  of other unfixed kernel bugs easier.
> +
> +	  There is no runtime or memory usage effect of this option
> +	  once the kernel has booted up - it's a one time check.
> +
> +	  If in doubt, say "Y".
> +
> +

Trivial nit: for consistency with the rest of the file, there should
only be one line space between options.

>  config DEBUG_SET_MODULE_RONX
>  	bool "Set loadable kernel module data as NX and text as RO"
>  	depends on MODULES
> diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
> index 8fc0957..6afd847 100644
> --- a/arch/arm64/include/asm/ptdump.h
> +++ b/arch/arm64/include/asm/ptdump.h
> @@ -42,5 +42,13 @@ static inline int ptdump_debugfs_register(struct ptdump_info *info,
>  	return 0;
>  }
>  #endif
> +void ptdump_check_wx(void);
> +#endif /* CONFIG_ARM64_PTDUMP_CORE */

... ah, here's the missing #endif from patch 1.

Assuming that gets sorted out, this looks good to me, and works on juno,
so FWIW:

Reviewed-by: Mark Rutland <mark.rutland@....com>
Tested-by: Mark Rutland <mark.rutland@....com>

[...]

> +static void note_prot_uxn(struct pg_state *st, unsigned long addr)
> +{
> +	if (!st->check_wx)
> +		return;
> +
> +	if ((st->current_prot & PTE_UXN) == PTE_UXN)
> +		return;
> +
> +	WARN_ONCE(1, "arm64/mm: Found non-UXN mapping at address %p/%pS\n",
> +		  (void *)st->start_address, (void *)st->start_address);
> +
> +	st->wx_pages += (addr - st->start_address) / PAGE_SIZE;
> +}

As a future thought (I've scope-creeped this enough with the UXN check),
there are some other checks that we could add, like verifying the AP
bits don't allow user data access. That might be worth considering, with
DEBUG_WX becoming a more general kernel page table sanity check.

Thanks,
Mark.