diff mbox series

[v2,8/9] riscv: introduce interfaces to patch kernel code

Message ID e2a42afbce47b364bf790b4cf8edf76235e48d53.1583741997.git.zong.li@sifive.com (mailing list archive)
State New, archived
Headers show
Series Support strict kernel memory permissions for security | expand

Commit Message

Zong Li March 9, 2020, 8:22 a.m. UTC 
On strict kernel memory permission, we couldn't patch code without
writable permission. Preserve two holes in fixmap area, so we can map
the kernel code temporarily to fixmap area, then patch the instructions.

We need two pages here because we support the compressed instruction, so
the instruction might be align to 2 bytes. When patching the 32-bit
length instruction which is 2 bytes alignment, it will across two pages.

Introduce two interfaces to patch kernel code:
riscv_patch_text_nosync:
 - patch code without synchronization, it's caller's responsibility to
   synchronize all CPUs if needed.
riscv_patch_text:
 - patch code and always synchronize with stop_machine()

Signed-off-by: Zong Li <zong.li@sifive.com>
---
 arch/riscv/include/asm/fixmap.h |   2 +
 arch/riscv/include/asm/patch.h  |  12 ++++
 arch/riscv/kernel/Makefile      |   4 +-
 arch/riscv/kernel/patch.c       | 124 ++++++++++++++++++++++++++++++++
 4 files changed, 141 insertions(+), 1 deletion(-)
 create mode 100644 arch/riscv/include/asm/patch.h
 create mode 100644 arch/riscv/kernel/patch.c

Comments

kernel test robot March 9, 2020, 12:49 p.m. UTC | #1 
Hi Zong,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.6-rc5]
[also build test WARNING on next-20200306]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Zong-Li/Support-strict-kernel-memory-permissions-for-security/20200309-172554
base:    2c523b344dfa65a3738e7039832044aa133c75fb
config: riscv-allnoconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/spinlock.h:318:0,
                    from arch/riscv/kernel/patch.c:6:
   arch/riscv/kernel/patch.c: In function 'riscv_insn_write':
   arch/riscv/kernel/patch.c:63:25: error: 'patch_lock' undeclared (first use in this function); did you mean 'patch_map'?
     raw_spin_lock_irqsave(&patch_lock, flags);
                            ^
   include/linux/spinlock_api_up.h:28:32: note: in definition of macro '___LOCK'
      do { __acquire(lock); (void)(lock); } while (0)
                                   ^~~~
>> include/linux/spinlock_api_up.h:40:31: note: in expansion of macro '__LOCK'
      do { local_irq_save(flags); __LOCK(lock); } while (0)
                                  ^~~~~~
>> include/linux/spinlock_api_up.h:68:45: note: in expansion of macro '__LOCK_IRQSAVE'
    #define _raw_spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
                                                ^~~~~~~~~~~~~~
>> include/linux/spinlock.h:272:3: note: in expansion of macro '_raw_spin_lock_irqsave'
      _raw_spin_lock_irqsave(lock, flags); \
      ^~~~~~~~~~~~~~~~~~~~~~
>> arch/riscv/kernel/patch.c:63:2: note: in expansion of macro 'raw_spin_lock_irqsave'
     raw_spin_lock_irqsave(&patch_lock, flags);
     ^~~~~~~~~~~~~~~~~~~~~
   arch/riscv/kernel/patch.c:63:25: note: each undeclared identifier is reported only once for each function it appears in
     raw_spin_lock_irqsave(&patch_lock, flags);
                            ^
   include/linux/spinlock_api_up.h:28:32: note: in definition of macro '___LOCK'
      do { __acquire(lock); (void)(lock); } while (0)
                                   ^~~~
>> include/linux/spinlock_api_up.h:40:31: note: in expansion of macro '__LOCK'
      do { local_irq_save(flags); __LOCK(lock); } while (0)
                                  ^~~~~~
>> include/linux/spinlock_api_up.h:68:45: note: in expansion of macro '__LOCK_IRQSAVE'
    #define _raw_spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
                                                ^~~~~~~~~~~~~~
>> include/linux/spinlock.h:272:3: note: in expansion of macro '_raw_spin_lock_irqsave'
      _raw_spin_lock_irqsave(lock, flags); \
      ^~~~~~~~~~~~~~~~~~~~~~
>> arch/riscv/kernel/patch.c:63:2: note: in expansion of macro 'raw_spin_lock_irqsave'
     raw_spin_lock_irqsave(&patch_lock, flags);
     ^~~~~~~~~~~~~~~~~~~~~
   arch/riscv/kernel/patch.c:66:25: error: 'FIX_TEXT_POKE1' undeclared (first use in this function)
      patch_map(addr + len, FIX_TEXT_POKE1);
                            ^~~~~~~~~~~~~~
   arch/riscv/kernel/patch.c:68:26: error: 'FIX_TEXT_POKE0' undeclared (first use in this function); did you mean 'FIX_TEXT_POKE1'?
     waddr = patch_map(addr, FIX_TEXT_POKE0);
                             ^~~~~~~~~~~~~~
                             FIX_TEXT_POKE1
--
   In file included from include/linux/spinlock.h:318:0,
                    from arch/riscv//kernel/patch.c:6:
   arch/riscv//kernel/patch.c: In function 'riscv_insn_write':
   arch/riscv//kernel/patch.c:63:25: error: 'patch_lock' undeclared (first use in this function); did you mean 'patch_map'?
     raw_spin_lock_irqsave(&patch_lock, flags);
                            ^
   include/linux/spinlock_api_up.h:28:32: note: in definition of macro '___LOCK'
      do { __acquire(lock); (void)(lock); } while (0)
                                   ^~~~
>> include/linux/spinlock_api_up.h:40:31: note: in expansion of macro '__LOCK'
      do { local_irq_save(flags); __LOCK(lock); } while (0)
                                  ^~~~~~
>> include/linux/spinlock_api_up.h:68:45: note: in expansion of macro '__LOCK_IRQSAVE'
    #define _raw_spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
                                                ^~~~~~~~~~~~~~
>> include/linux/spinlock.h:272:3: note: in expansion of macro '_raw_spin_lock_irqsave'
      _raw_spin_lock_irqsave(lock, flags); \
      ^~~~~~~~~~~~~~~~~~~~~~
   arch/riscv//kernel/patch.c:63:2: note: in expansion of macro 'raw_spin_lock_irqsave'
     raw_spin_lock_irqsave(&patch_lock, flags);
     ^~~~~~~~~~~~~~~~~~~~~
   arch/riscv//kernel/patch.c:63:25: note: each undeclared identifier is reported only once for each function it appears in
     raw_spin_lock_irqsave(&patch_lock, flags);
                            ^
   include/linux/spinlock_api_up.h:28:32: note: in definition of macro '___LOCK'
      do { __acquire(lock); (void)(lock); } while (0)
                                   ^~~~
>> include/linux/spinlock_api_up.h:40:31: note: in expansion of macro '__LOCK'
      do { local_irq_save(flags); __LOCK(lock); } while (0)
                                  ^~~~~~
>> include/linux/spinlock_api_up.h:68:45: note: in expansion of macro '__LOCK_IRQSAVE'
    #define _raw_spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
                                                ^~~~~~~~~~~~~~
>> include/linux/spinlock.h:272:3: note: in expansion of macro '_raw_spin_lock_irqsave'
      _raw_spin_lock_irqsave(lock, flags); \
      ^~~~~~~~~~~~~~~~~~~~~~
   arch/riscv//kernel/patch.c:63:2: note: in expansion of macro 'raw_spin_lock_irqsave'
     raw_spin_lock_irqsave(&patch_lock, flags);
     ^~~~~~~~~~~~~~~~~~~~~
   arch/riscv//kernel/patch.c:66:25: error: 'FIX_TEXT_POKE1' undeclared (first use in this function)
      patch_map(addr + len, FIX_TEXT_POKE1);
                            ^~~~~~~~~~~~~~
   arch/riscv//kernel/patch.c:68:26: error: 'FIX_TEXT_POKE0' undeclared (first use in this function); did you mean 'FIX_TEXT_POKE1'?
     waddr = patch_map(addr, FIX_TEXT_POKE0);
                             ^~~~~~~~~~~~~~
                             FIX_TEXT_POKE1

vim +/_raw_spin_lock_irqsave +272 include/linux/spinlock.h

b8e6ec865fd1d8 Linus Torvalds  2006-11-26  268  
c2f21ce2e31286 Thomas Gleixner 2009-12-02  269  #define raw_spin_lock_irqsave(lock, flags)		\
3f307891ce0e7b Steven Rostedt  2008-07-25  270  	do {						\
3f307891ce0e7b Steven Rostedt  2008-07-25  271  		typecheck(unsigned long, flags);	\
9c1721aa4994f6 Thomas Gleixner 2009-12-03 @272  		_raw_spin_lock_irqsave(lock, flags);	\
3f307891ce0e7b Steven Rostedt  2008-07-25  273  	} while (0)
ef12f10994281e Thomas Gleixner 2009-11-07  274  

:::::: The code at line 272 was first introduced by commit
:::::: 9c1721aa4994f6625decbd915241f3a94ee2fe67 locking: Cleanup the name space completely

:::::: TO: Thomas Gleixner <tglx@linutronix.de>
:::::: CC: Thomas Gleixner <tglx@linutronix.de>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot March 9, 2020, 1:12 p.m. UTC | #2 
Hi Zong,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v5.6-rc5]
[also build test ERROR on next-20200306]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Zong-Li/Support-strict-kernel-memory-permissions-for-security/20200309-172554
base:    2c523b344dfa65a3738e7039832044aa133c75fb
config: riscv-nommu_virt_defconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/riscv//kernel/patch.c:6:0:
   arch/riscv//kernel/patch.c: In function 'riscv_insn_write':
>> arch/riscv//kernel/patch.c:63:25: error: 'patch_lock' undeclared (first use in this function); did you mean 'patch_map'?
     raw_spin_lock_irqsave(&patch_lock, flags);
                            ^
   include/linux/spinlock.h:250:34: note: in definition of macro 'raw_spin_lock_irqsave'
      flags = _raw_spin_lock_irqsave(lock); \
                                     ^~~~
   arch/riscv//kernel/patch.c:63:25: note: each undeclared identifier is reported only once for each function it appears in
     raw_spin_lock_irqsave(&patch_lock, flags);
                            ^
   include/linux/spinlock.h:250:34: note: in definition of macro 'raw_spin_lock_irqsave'
      flags = _raw_spin_lock_irqsave(lock); \
                                     ^~~~
>> arch/riscv//kernel/patch.c:66:25: error: 'FIX_TEXT_POKE1' undeclared (first use in this function)
      patch_map(addr + len, FIX_TEXT_POKE1);
                            ^~~~~~~~~~~~~~
>> arch/riscv//kernel/patch.c:68:26: error: 'FIX_TEXT_POKE0' undeclared (first use in this function); did you mean 'FIX_TEXT_POKE1'?
     waddr = patch_map(addr, FIX_TEXT_POKE0);
                             ^~~~~~~~~~~~~~
                             FIX_TEXT_POKE1

vim +63 arch/riscv//kernel/patch.c

    55	
    56	static int __kprobes riscv_insn_write(void *addr, const void *insn, size_t len)
    57	{
    58		void *waddr = addr;
    59		bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
    60		unsigned long flags = 0;
    61		int ret;
    62	
  > 63		raw_spin_lock_irqsave(&patch_lock, flags);
    64	
    65		if (across_pages)
  > 66			patch_map(addr + len, FIX_TEXT_POKE1);
    67	
  > 68		waddr = patch_map(addr, FIX_TEXT_POKE0);
    69	
    70		ret = probe_kernel_write(waddr, insn, len);
    71	
    72		patch_unmap(FIX_TEXT_POKE0);
    73	
    74		if (across_pages)
    75			patch_unmap(FIX_TEXT_POKE1);
    76	
    77		raw_spin_unlock_irqrestore(&patch_lock, flags);
    78	
    79		return ret;
    80	}
    81	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/fixmap.h b/arch/riscv/include/asm/fixmap.h
index 42d2c42f3cc9..2368d49eb4ef 100644
--- a/arch/riscv/include/asm/fixmap.h
+++ b/arch/riscv/include/asm/fixmap.h
@@ -27,6 +27,8 @@  enum fixed_addresses {
 	FIX_FDT = FIX_FDT_END + FIX_FDT_SIZE / PAGE_SIZE - 1,
 	FIX_PTE,
 	FIX_PMD,
+	FIX_TEXT_POKE1,
+	FIX_TEXT_POKE0,
 	FIX_EARLYCON_MEM_BASE,
 	__end_of_fixed_addresses
 };
diff --git a/arch/riscv/include/asm/patch.h b/arch/riscv/include/asm/patch.h
new file mode 100644
index 000000000000..b5918a6e0615
--- /dev/null
+++ b/arch/riscv/include/asm/patch.h
@@ -0,0 +1,12 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2020 SiFive
+ */
+
+#ifndef _ASM_RISCV_PATCH_H
+#define _ASM_RISCV_PATCH_H
+
+int riscv_patch_text_nosync(void *addr, const void *insns, size_t len);
+int riscv_patch_text(void *addr, u32 insn);
+
+#endif /* _ASM_RISCV_PATCH_H */
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index f40205cb9a22..d189bd3d8501 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -4,7 +4,8 @@ 
 #
 
 ifdef CONFIG_FTRACE
-CFLAGS_REMOVE_ftrace.o = -pg
+CFLAGS_REMOVE_ftrace.o	= -pg
+CFLAGS_REMOVE_patch.o	= -pg
 endif
 
 extra-y += head.o
@@ -26,6 +27,7 @@  obj-y	+= traps.o
 obj-y	+= riscv_ksyms.o
 obj-y	+= stacktrace.o
 obj-y	+= cacheinfo.o
+obj-y	+= patch.o
 obj-$(CONFIG_MMU) += vdso.o vdso/
 
 obj-$(CONFIG_RISCV_M_MODE)	+= clint.o
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
new file mode 100644
index 000000000000..1c61a8595839
--- /dev/null
+++ b/arch/riscv/kernel/patch.c
@@ -0,0 +1,124 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 SiFive
+ */
+
+#include <linux/spinlock.h>
+#include <linux/mm.h>
+#include <linux/uaccess.h>
+#include <linux/stop_machine.h>
+#include <asm/kprobes.h>
+#include <asm/cacheflush.h>
+#include <asm/fixmap.h>
+
+struct riscv_insn_patch {
+	void *addr;
+	u32 insn;
+	atomic_t cpu_count;
+};
+
+#ifdef CONFIG_MMU
+static DEFINE_RAW_SPINLOCK(patch_lock);
+
+static void __kprobes *patch_map(void *addr, int fixmap)
+{
+	uintptr_t uintaddr = (uintptr_t) addr;
+	struct page *page;
+
+	if (core_kernel_text(uintaddr))
+		page = phys_to_page(__pa_symbol(addr));
+	else if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
+		page = vmalloc_to_page(addr);
+	else
+		return addr;
+
+	BUG_ON(!page);
+
+	return (void *)set_fixmap_offset(fixmap, page_to_phys(page) +
+					 (uintaddr & ~PAGE_MASK));
+}
+
+static void __kprobes patch_unmap(int fixmap)
+{
+	clear_fixmap(fixmap);
+}
+#else
+static void __kprobes *patch_map(void *addr, int fixmap)
+{
+	return addr;
+}
+
+static void __kprobes patch_unmap(int fixmap)
+{
+}
+#endif /* CONFIG_MMU */
+
+static int __kprobes riscv_insn_write(void *addr, const void *insn, size_t len)
+{
+	void *waddr = addr;
+	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
+	unsigned long flags = 0;
+	int ret;
+
+	raw_spin_lock_irqsave(&patch_lock, flags);
+
+	if (across_pages)
+		patch_map(addr + len, FIX_TEXT_POKE1);
+
+	waddr = patch_map(addr, FIX_TEXT_POKE0);
+
+	ret = probe_kernel_write(waddr, insn, len);
+
+	patch_unmap(FIX_TEXT_POKE0);
+
+	if (across_pages)
+		patch_unmap(FIX_TEXT_POKE1);
+
+	raw_spin_unlock_irqrestore(&patch_lock, flags);
+
+	return ret;
+}
+
+int __kprobes riscv_patch_text_nosync(void *addr, const void *insns, size_t len)
+{
+	u32 *tp = addr;
+	int ret;
+
+	ret = riscv_insn_write(tp, insns, len);
+
+	if (!ret)
+		flush_icache_range((uintptr_t) tp, (uintptr_t) tp + len);
+
+	return ret;
+}
+
+static int __kprobes riscv_patch_text_cb(void *data)
+{
+	struct riscv_insn_patch *patch = data;
+	int ret = 0;
+
+	if (atomic_inc_return(&patch->cpu_count) == 1) {
+		ret =
+		    riscv_patch_text_nosync(patch->addr, &patch->insn,
+					    GET_INSN_LENGTH(patch->insn));
+		atomic_inc(&patch->cpu_count);
+	} else {
+		while (atomic_read(&patch->cpu_count) <= num_online_cpus())
+			cpu_relax();
+		smp_mb();
+	}
+
+	return ret;
+}
+
+int __kprobes riscv_patch_text(void *addr, u32 insn)
+{
+	struct riscv_insn_patch patch = {
+		.addr = addr,
+		.insn = insn,
+		.cpu_count = ATOMIC_INIT(0),
+	};
+
+	return stop_machine_cpuslocked(riscv_patch_text_cb,
+				       &patch, cpu_online_mask);
+}