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

On Thu Aug 5, 2021 at 4:09 AM CDT, Christophe Leroy wrote:

Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :

quoted

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 ?

Hmm a common patch/poke kernel API is probably out of scope for this
series? I do agree though - since you suggested splitting the series
maybe that's something I can add along with the LKDTM patches.

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.

I followed the approach in lkdtm_DOUBLE_FAULT and others in
drivers/misc/lkdtm/bugs.c. I suppose it doesn't hurt to always build the
test irrespective of CONFIG_STRICT_KERNEL_RWX.

Christophe