Thread (24 messages) flat view 24 messages, 3 authors, 2021-08-11

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

From: Christophe Leroy <hidden>
Date: 2021-08-05 09:09:35
Also in: linux-hardening


Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :
quoted hunk ↗ jump to hunk
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 <redacted>
---
  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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help