linux-hardening - Re: [PATCH v5 4/8] lkdtm/x86_64: Add test to hijack a patch mapping

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <95de5ec5-8d48-c969-3c9f-966561f9f58e@csgroup.eu>
Date:   Thu, 5 Aug 2021 11:09:06 +0200
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     "Christopher M. Riedl" <cmr@...ux.ibm.com>,
        linuxppc-dev@...ts.ozlabs.org
Cc:     keescook@...omium.org, peterz@...radead.org, x86@...nel.org,
        npiggin@...il.com, linux-hardening@...r.kernel.org,
        tglx@...utronix.de, dja@...ens.net
Subject: Re: [PATCH v5 4/8] lkdtm/x86_64: Add test to hijack a patch mapping



Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :
> A previous commit implemented an LKDTM test on powerpc to exploit the
> temporary mapping established when patching code with STRICT_KERNEL_RWX
> enabled. Extend the test to work on x86_64 as well.
> 
> Signed-off-by: Christopher M. Riedl <cmr@...ux.ibm.com>
> ---
>   drivers/misc/lkdtm/perms.c | 26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 39e7456852229..41e87e5f9cc86 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -224,7 +224,7 @@ void lkdtm_ACCESS_NULL(void)
>   }
>   
>   #if (IS_BUILTIN(CONFIG_LKDTM) && defined(CONFIG_STRICT_KERNEL_RWX) && \
> -	defined(CONFIG_PPC))
> +	(defined(CONFIG_PPC) || defined(CONFIG_X86_64)))
>   /*
>    * This is just a dummy location to patch-over.
>    */
> @@ -233,12 +233,25 @@ static void patching_target(void)
>   	return;
>   }
>   
> -#include <asm/code-patching.h>
>   const u32 *patch_site = (const u32 *)&patching_target;
>   
> +#ifdef CONFIG_PPC
> +#include <asm/code-patching.h>
> +#endif
> +
> +#ifdef CONFIG_X86_64
> +#include <asm/text-patching.h>
> +#endif
> +
>   static inline int lkdtm_do_patch(u32 data)
>   {
> +#ifdef CONFIG_PPC
>   	return patch_instruction((u32 *)patch_site, ppc_inst(data));
> +#endif
> +#ifdef CONFIG_X86_64
> +	text_poke((void *)patch_site, &data, sizeof(u32));
> +	return 0;
> +#endif
>   }
>   
>   static inline u32 lkdtm_read_patch_site(void)
> @@ -249,11 +262,16 @@ static inline u32 lkdtm_read_patch_site(void)
>   /* Returns True if the write succeeds */
>   static inline bool lkdtm_try_write(u32 data, u32 *addr)
>   {
> +#ifdef CONFIG_PPC
>   	__put_kernel_nofault(addr, &data, u32, err);
>   	return true;
>   
>   err:
>   	return false;
> +#endif
> +#ifdef CONFIG_X86_64
> +	return !__put_user(data, addr);
> +#endif
>   }
>   
>   static int lkdtm_patching_cpu(void *data)
> @@ -346,8 +364,8 @@ void lkdtm_HIJACK_PATCH(void)
>   
>   void lkdtm_HIJACK_PATCH(void)
>   {
> -	if (!IS_ENABLED(CONFIG_PPC))
> -		pr_err("XFAIL: this test only runs on powerpc\n");
> +	if (!IS_ENABLED(CONFIG_PPC) && !IS_ENABLED(CONFIG_X86_64))
> +		pr_err("XFAIL: this test only runs on powerpc and x86_64\n");
>   	if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
>   		pr_err("XFAIL: this test requires CONFIG_STRICT_KERNEL_RWX\n");
>   	if (!IS_BUILTIN(CONFIG_LKDTM))
> 

Instead of spreading arch specific stuff into LKDTM, wouldn't it make sence to define common a 
common API ? Because the day another arch like arm64 implements it own approach, do we add specific 
functions again and again into LKDTM ?

Also, I find it odd to define tests only when they can succeed. For other tests like 
ACCESS_USERSPACE, they are there all the time, regardless of whether we have selected 
CONFIG_PPC_KUAP or not. I think it should be the same here, have it all there time, if 
CONFIG_STRICT_KERNEL_RWX is selected the test succeeds otherwise it fails, but it is always there.

Christophe