[PATCH 0/3] arm64:msr: Add MSR driver

[PATCH 0/3] arm64:msr: Add MSR driver

[Rongwei Wang]

Hi

MSR ARM driver aims to provide interfacs for user to read or write data to all
system registers. Its functions is same as MSR driver (x86 platform). It mainly
depends on kprobe and undef exception to read or write system registers
dynamicly.

In addition, this module create interfaces for each core. We have tested in the
virtual machine using tree command:

$ tree /dev/cpu
/dev/cpu
|-- 0
|   `-- msr
|-- 1
|   `-- msr
|-- 2
|   `-- msr
|-- 3
|   `-- msr
|-- 4
|   `-- msr
|-- 5
|   `-- msr
|-- 6
|   `-- msr
`-- 7
    `-- msr

8 directories, 8 files

The interfaces of this module is same as MSR module in user space, and to solve
the problem that ARM platform has no similar MSR module. Using this interface,
we did some pressure tests to test the stability and security of MSR driver. The
test results show that the driver will not cause system panic if various
illegal values and multithreading concurrent access are passed through the
interface.

We also designed a user space tool: system-register-tools. We have open
sourced this tool, which link as follows:
	https://github.com/alibaba/system-register-tools
In this tools, we provide two command: rdasr and wrasr, the aboving MSR driver
has been tested by:

$ rdasr -t
  0: OSDTRRX_EL1         : UNDEFINED or unreadable!
  1: DBGBVR0_EL1         : 0x0
  2: DBGBCR0_EL1         : 0x1e0
  3: DBGWVR0_EL1         : 0x0
  4: DBGWCR0_EL1         : 0x0
  5: DBGBVR1_EL1         : 0x0
  6: DBGBCR1_EL1         : 0x1e0
  7: DBGWVR1_EL1         : 0x0
  8: DBGWCR1_EL1         : 0x0
  9: MDCCINT_EL1         : 0x0
 10: MDSCR_EL1           : 0x1000
 11: DBGBVR2_EL1         : 0x0
...
...
598: ICC_IGRPEN1_EL3     : UNDEFINED or unreadable!
599: TPIDR_EL3           : UNDEFINED or unreadable!
600: SCXTNUM_EL3         : UNDEFINED or unreadable!
601: CNTPS_TVAL_EL1      : UNDEFINED or unreadable!
602: CNTPS_CTL_EL1       : UNDEFINED or unreadable!
603: CNTPS_CVAL_EL1      : UNDEFINED or unreadable!
604: CNTPS_CVAL_EL1      : UNDEFINED or unreadable!

The test ended and no system exception occurred!
Undefined or unreadable registers: 409
Readable registers: 196

The above is a test of more than 600 system registers, and no system exception
occurred

Rongwei Wang (3):
  arm64:insn: Export the symbol to modify code text
  arm64:msr: Introduce MSR ARM driver
  arm64:msr: Enable MSR ARM driver

 arch/arm64/Kconfig               |   9 +
 arch/arm64/configs/defconfig     |   1 +
 arch/arm64/include/asm/msr_arm.h |  80 ++++++++
 arch/arm64/kernel/Makefile       |   3 +-
 arch/arm64/kernel/insn.c         |   1 +
 arch/arm64/kernel/msr_arm.c      | 406 +++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/msr_smp.c      | 297 ++++++++++++++++++++++++++++
 7 files changed, 796 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/include/asm/msr_arm.h
 create mode 100644 arch/arm64/kernel/msr_arm.c
 create mode 100644 arch/arm64/kernel/msr_smp.c

-- 
1.8.3.1

[PATCH 1/3] arm64:insn: Export symbols for MSR ARM driver

[Rongwei Wang]

In order to use the MSR-ARM driver in module and built-in ways, we need export
following symbols.

The MSR-ARM driver depends on function aarch64_insn_patch_text to modify the
text at runtime, and function aarch64_insn_read to read one instruction in the
specified address.

In addition, we need register_undef_hook and unregister_undef_hook to handle
the low probability undefined exception which triggered only by MSR-ARM driver.

Signed-off-by: Rongwei Wang <rongwei.wang@linux.alibaba.com>
---
 arch/arm64/kernel/insn.c  | 2 ++
 arch/arm64/kernel/traps.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index 6c0de2f..67a1f78 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -135,6 +135,7 @@ int __kprobes aarch64_insn_read(void *addr, u32 *insnp)
 
 	return ret;
 }
+EXPORT_SYMBOL(aarch64_insn_read);
 
 static int __kprobes __aarch64_insn_write(void *addr, __le32 insn)
 {
@@ -247,6 +248,7 @@ int __kprobes aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt)
 	return stop_machine_cpuslocked(aarch64_insn_patch_text_cb, &patch,
 				       cpu_online_mask);
 }
+EXPORT_SYMBOL(aarch64_insn_patch_text);
 
 static int __kprobes aarch64_get_imm_shift_mask(enum aarch64_insn_imm_type type,
 						u32 *maskp, int *shiftp)
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 8af4e0e..c76efa5 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -292,6 +292,7 @@ void register_undef_hook(struct undef_hook *hook)
 	list_add(&hook->node, &undef_hook);
 	raw_spin_unlock_irqrestore(&undef_lock, flags);
 }
+EXPORT_SYMBOL(register_undef_hook);
 
 void unregister_undef_hook(struct undef_hook *hook)
 {
@@ -301,6 +302,7 @@ void unregister_undef_hook(struct undef_hook *hook)
 	list_del(&hook->node);
 	raw_spin_unlock_irqrestore(&undef_lock, flags);
 }
+EXPORT_SYMBOL(unregister_undef_hook);
 
 static int call_undef_hook(struct pt_regs *regs)
 {
-- 
1.8.3.1

[PATCH 2/3] arm64:msr: Introduce MSR ARM driver

[Rongwei Wang]

The function of MSR-ARM driver is similar to that of MSR module under
x86. It is used to read and write system registers in user mode, which is easy
to debug.
In the implementation of MSR-ARM module, because the current aarch64
architecture lacks the support of rdmsr and wrmsr instructions, we have to
modify the code segment at runtime to realize the reading and writing of
arbitrary registers.
In terms of security, because modifying the code segment may generate undefined
instruction exceptions, we use the existing undefined instruction exception
hook function registration interface of the kernel to shield the exceptions
only generated by MSR-ARM.

Meanwhile, we designed a user space tool: system-register-tools, which function
similar to msr-tools under x86. We have open sourced this tool in Github, link
as follows: https://github.com/alibaba/system-register-tools.

Signed-off-by: Rongwei Wang <rongwei.wang@linux.alibaba.com>
---
 arch/arm64/Kconfig               |   9 +
 arch/arm64/include/asm/msr_arm.h |  75 ++++++++
 arch/arm64/kernel/Makefile       |   3 +-
 arch/arm64/kernel/msr_arm.c      | 399 +++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/msr_smp.c      | 249 ++++++++++++++++++++++++
 5 files changed, 734 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/include/asm/msr_arm.h
 create mode 100644 arch/arm64/kernel/msr_arm.c
 create mode 100644 arch/arm64/kernel/msr_smp.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1515f6f..8077ff6 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1688,6 +1688,15 @@ config ARM64_MTE
 
 endmenu
 
+config ARM64_MSR
+	tristate "/dev/cpu/*/msr - AARCH64 Model-specific register support"
+	help
+	  This device gives privileged processes access to the arm64
+	  Model-Specific Registers (MSRs).  It is a character device with
+	  major 202 and minors 0 to <NR_CPUS-1> for /dev/cpu/0/msr to
+	  /dev/cpu/<NR_CPUS-1>/msr. MSR accesses are directed to a specific CPU on
+	  multi-processor systems.
+
 config ARM64_SVE
 	bool "ARM Scalable Vector Extension support"
 	default y
diff --git a/arch/arm64/include/asm/msr_arm.h b/arch/arm64/include/asm/msr_arm.h
new file mode 100644
index 0000000..9b52ef2
--- /dev/null
+++ b/arch/arm64/include/asm/msr_arm.h
@@ -0,0 +1,75 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_MSR_ARM_H
+#define _ASM_MSR_ARM_H
+
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/fcntl.h>
+#include <linux/init.h>
+#include <linux/poll.h>
+#include <linux/smp.h>
+#include <linux/major.h>
+#include <linux/fs.h>
+#include <linux/device.h>
+#include <linux/cpu.h>
+#include <linux/notifier.h>
+#include <linux/uaccess.h>
+#include <linux/gfp.h>
+#include <linux/kernel.h>
+#include <asm/cpufeature.h>
+#include <asm/cpu.h>
+#include <asm/sysreg.h>
+#include <asm/insn.h>
+#include <asm/traps.h>
+#include <asm/atomic.h>
+#include <linux/printk.h>
+
+/* the opcode of mrs/msr instruction */
+#define AARCH64_MRS_INSN (0xd5200000)
+#define AARCH64_MSR_INSN (0xd5000000)
+#define INSN_REG_MASK (0x0000001f)
+
+/*
+ * the max number of each code
+ * in system registers
+ */
+#define MAX_OP0 3
+#define MAX_OP1 7
+#define MAX_OP2 7
+#define MAX_CN 15
+#define MAX_CM 15
+
+#define MSR_DEBUG_REG 0x02
+#define MSR_NON_DEBUG_REG 0x03
+
+extern atomic_t msr_flags;
+extern atomic_t mrs_flags;
+
+struct msr {
+	union {
+		struct {
+			u32 l;
+			u32 h;
+		};
+		u64 q;
+	};
+};
+
+struct msr_info {
+	u32 msr_no;
+	u32 opt;
+	struct msr reg;
+	struct msr *msrs;
+	int err;
+};
+
+int aarch64_modify_read_text(u32 opcode);
+int aarch64_modify_write_text(u32 opcode);
+
+u32 *get_read_insn_addr(void);
+u32 *get_write_insn_addr(void);
+
+int rdmsr_safe_on_cpu_aarch64(u32 cpu, u32 opt, u32 *l, u32 *h);
+int wrmsr_safe_on_cpu_aarch64(u32 cpu, u32 opt, u32 l, u32 h);
+
+#endif /* _ASM_MSR_ARM_H */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index bbaf0bc..0fd2eff 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -17,7 +17,7 @@ obj-y			:= debug-monitors.o entry.o irq.o fpsimd.o		\
 			   return_address.o cpuinfo.o cpu_errata.o		\
 			   cpufeature.o alternative.o cacheinfo.o		\
 			   smp.o smp_spin_table.o topology.o smccc-call.o	\
-			   syscall.o proton-pack.o
+			   syscall.o proton-pack.o msr_smp.o
 
 targets			+= efi-entry.o
 
@@ -60,6 +60,7 @@ obj-$(CONFIG_ARM_SDE_INTERFACE)		+= sdei.o
 obj-$(CONFIG_ARM64_PTR_AUTH)		+= pointer_auth.o
 obj-$(CONFIG_SHADOW_CALL_STACK)		+= scs.o
 obj-$(CONFIG_ARM64_MTE)			+= mte.o
+obj-$(CONFIG_ARM64_MSR)			+= msr_arm.o
 
 obj-y					+= vdso/ probes/
 obj-$(CONFIG_COMPAT_VDSO)		+= vdso32/
diff --git a/arch/arm64/kernel/msr_arm.c b/arch/arm64/kernel/msr_arm.c
new file mode 100644
index 0000000..4dec683
--- /dev/null
+++ b/arch/arm64/kernel/msr_arm.c
@@ -0,0 +1,399 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * ARM system register access device
+ *
+ * This device is accessed by read() to the appropriate register number
+ * and then read/write in chunks of 8 bytes.  A larger size means multiple
+ * reads or writes of the same register.
+ *
+ * This driver uses /dev/cpu/%d/msr where %d is the minor number, and on
+ * an SMP box will direct the access to CPU %d.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/fcntl.h>
+#include <linux/init.h>
+#include <linux/poll.h>
+#include <linux/smp.h>
+#include <linux/major.h>
+#include <linux/fs.h>
+#include <linux/device.h>
+#include <linux/cpu.h>
+#include <linux/notifier.h>
+#include <linux/uaccess.h>
+#include <linux/gfp.h>
+#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <asm/cpufeature.h>
+#include <asm/cpu.h>
+#include <asm/sysreg.h>
+#include <asm/insn.h>
+#include <asm/traps.h>
+#include <asm/mmu.h>
+#include <asm/barrier.h>
+#include <asm/fixmap.h>
+#include <linux/printk.h>
+#include <asm/msr_arm.h>
+
+static struct class *msr_class;
+static enum cpuhp_state cpuhp_msr_state;
+
+static int hookers_mrs(struct pt_regs *regs, u32 insn)
+{
+	/* judge whether this exception is generated by MRS instruction */
+	if (atomic_read(&mrs_flags) &&
+		(regs->pc == (u64)get_read_insn_addr())) {
+		/* skip undef instruction and jump */
+		regs->pc += 2*AARCH64_INSN_SIZE;
+		pr_warn("undef exception has been ignored!\n");
+
+		return 0;
+	} else {
+		/* NOTE: NOT an exception from MSR driver, must return 1!!! */
+		return 1;
+	}
+}
+
+static int hookers_msr(struct pt_regs *regs, u32 insn)
+{
+	/* judge whether this exception is generated by MSR instruction */
+	if (atomic_read(&msr_flags) &&
+		(regs->pc == (u64)get_write_insn_addr())) {
+		/* skip undef instruction and jump */
+		regs->pc += 2*AARCH64_INSN_SIZE;
+		pr_warn("undef exception has been ignored!\n");
+
+		return 0;
+	} else {
+		/* NOTE: NOT an exception from MSR driver, must return 1!!! */
+		return 1;
+	}
+}
+
+/*
+ * The following four variables is used to judge whether the undefine exception
+ * is generated by the MSR and MRS instruction in EL1 and EL2.
+ */
+static struct undef_hook mrs_hook_el1 = {
+	.instr_mask = 0xfff00000,
+	.instr_val  = 0xd5300000,
+	.pstate_mask = PSR_MODE_MASK,
+	.pstate_val = PSR_MODE_EL1h,
+	.fn = hookers_mrs,
+};
+
+static struct undef_hook msr_hook_el1 = {
+	.instr_mask = 0xfff00000,
+	.instr_val  = 0xd5100000,
+	.pstate_mask = PSR_MODE_MASK,
+	.pstate_val = PSR_MODE_EL1h,
+	.fn = hookers_msr,
+};
+
+static struct undef_hook mrs_hook_el2 = {
+	.instr_mask = 0xfff00000,
+	.instr_val  = 0xd5300000,
+	.pstate_mask = PSR_MODE_MASK,
+	.pstate_val = PSR_MODE_EL2h,
+	.fn = hookers_mrs,
+};
+
+static struct undef_hook msr_hook_el2 = {
+	.instr_mask = 0xfff00000,
+	.instr_val  = 0xd5100000,
+	.pstate_mask = PSR_MODE_MASK,
+	.pstate_val = PSR_MODE_EL2h,
+	.fn = hookers_msr,
+};
+
+/*
+ * ARMv8 ARM reserves the following encoding for system registers:
+ * (Ref: ARMv8 ARM, Section: "System instruction class encoding overview",
+ *  C5.2, version:ARM DDI 0487A.f)
+ *	[20-19] : Op0
+ *	[18-16] : Op1
+ *	[15-12] : CRn
+ *	[11-8]  : CRm
+ *	[7-5]   : Op2
+ *
+ * make MSR/MRS instruction
+ */
+static u32 aarch64_insn_mrs_gen(u32 op0, u32 op1, u32 crn, u32 crm, u32 op2, u32 rt)
+{
+	return (AARCH64_MRS_INSN | sys_reg(op0, op1, crn, crm, op2) | rt);
+}
+
+static u32 aarch64_insn_msr_gen(u32 op0, u32 op1, u32 crn, u32 crm, u32 op2, u32 rt)
+{
+	return (AARCH64_MSR_INSN | sys_reg(op0, op1, crn, crm, op2) | rt);
+}
+
+/*
+ * check the regcode legal
+ */
+static int aarch64_register_check(u32 reg)
+{
+	unsigned int op0, op1, cn, cm, op2;
+	u32 max_reg;
+	int ret;
+
+	max_reg = sys_reg(MAX_OP0, MAX_OP1, MAX_CN, MAX_CM, MAX_OP2);
+	if (reg & ~max_reg) {
+		/* illegal regcode */
+		return -EFAULT;
+	}
+
+	op0 = sys_reg_Op0(reg);
+	op1 = sys_reg_Op1(reg);
+	cn = sys_reg_CRn(reg);
+	cm = sys_reg_CRm(reg);
+	op2 = sys_reg_Op2(reg);
+
+	/*
+	 * for system registers, their value of op0: 0b10 or 0b11.
+	 */
+	if (op0 != MSR_DEBUG_REG && op0 != MSR_NON_DEBUG_REG) {
+		/* NOT support */
+		return -EFAULT;
+	}
+
+	if (op0 <= MAX_OP0 && op1 <= MAX_OP1 && op2 <= MAX_OP2
+			&& cn <= MAX_CN && cm <= MAX_CM) {
+		/* legal regcode */
+		ret = 0;
+	} else {
+		/* illegal regcode */
+		ret = -EFAULT;
+	}
+	return ret;
+}
+
+/*
+ * Before reading and writing register, modify the instruction on
+ * corresponding address
+ */
+static long msr_insn_smc(unsigned int ioc, unsigned long arg)
+{
+	u32 insnp = 0, insn = 0;
+	u32 reg = arg;
+	int err = 0;
+	unsigned int op0, op1, cn, cm, op2;
+
+	op0 = sys_reg_Op0(reg);
+	op1 = sys_reg_Op1(reg);
+	cn  = sys_reg_CRn(reg);
+	cm  = sys_reg_CRm(reg);
+	op2 = sys_reg_Op2(reg);
+	err = aarch64_register_check(reg);
+	if (err) {
+		/* illegal register */
+		return err;
+	}
+
+	switch (ioc) {
+	case 0x00:
+		err = aarch64_insn_read((void *)get_read_insn_addr(), &insnp);
+		if (err)
+			return err;
+		insn = aarch64_insn_mrs_gen(op0, op1, cn, cm, op2,
+				insnp & INSN_REG_MASK);
+		err = aarch64_modify_read_text(insn);
+		break;
+	case 0x01:
+		err = aarch64_insn_read((void *)get_write_insn_addr(), &insnp);
+		if (err)
+			return err;
+		insn = aarch64_insn_msr_gen(op0, op1, cn, cm, op2,
+				insnp & INSN_REG_MASK);
+		err = aarch64_modify_write_text(insn);
+		break;
+	default:
+		err = -ENOTTY;
+		break;
+	}
+
+	return err;
+}
+
+static ssize_t msr_read(struct file *file, char __user *buf,
+			size_t count, loff_t *ppos)
+{
+	u32 __user *tmp = (u32 __user *) buf;
+	u32 data[2];
+	u32 reg = *ppos;
+	int cpu = iminor(file_inode(file));
+	int err;
+	ssize_t bytes = 0;
+
+	err = msr_insn_smc(0x00, reg);
+	if (err != 0) {
+		/* illegal register */
+		return err;
+	}
+
+	if (count % 8)
+		return -EINVAL;	/* Invalid chunk size */
+	for (; count; count -= 8) {
+		err = rdmsr_safe_on_cpu_aarch64(cpu, 1, &data[0], &data[1]);
+		if (err)
+			break;
+		if (copy_to_user(tmp, &data, 8)) {
+			pr_err("copy error");
+			err = -EFAULT;
+			break;
+		}
+		tmp += 2;
+		bytes += 8;
+	}
+
+	return bytes ? bytes : err;
+}
+
+static ssize_t msr_write(struct file *file, const char __user *buf,
+			 size_t count, loff_t *ppos)
+{
+	const u32 __user *tmp = (const u32 __user *)buf;
+	u32 data[2];
+	u32 reg = *ppos;
+	int cpu = iminor(file_inode(file));
+	int err;
+	ssize_t bytes = 0;
+
+	err = msr_insn_smc(0x01, reg);
+	if (err != 0) {
+		/* illegal register */
+		return err;
+	}
+
+	if (count % 8)
+		return -EINVAL;	/* Invalid chunk size */
+
+	if (copy_from_user(&data, tmp, 8)) {
+		err = -EFAULT;
+		return err;
+	}
+	err = wrmsr_safe_on_cpu_aarch64(cpu, 1, data[0], data[1]);
+	if (err)
+		return err;
+	bytes += 8;
+
+	return bytes ? bytes : err;
+}
+
+static int msr_open(struct inode *inode, struct file *file)
+{
+	unsigned int cpu = iminor(file_inode(file));
+	/* TODO */
+	struct cpuinfo_arm64 *c;
+
+	if (!capable(CAP_SYS_RAWIO))
+		return -EPERM;
+
+	if (cpu >= nr_cpu_ids || !cpu_online(cpu))
+		return -ENXIO;	/* No such CPU */
+
+	c = &per_cpu(cpu_data, cpu);
+	return 0;
+}
+
+/*
+ * File operations support
+ */
+static const struct file_operations msr_fops = {
+	.owner = THIS_MODULE,
+	.llseek = no_seek_end_llseek,
+	.read = msr_read,
+	.write = msr_write,
+	.open = msr_open,
+};
+
+static int msr_device_create(unsigned int cpu)
+{
+	struct device *dev;
+
+	dev = device_create(msr_class, NULL, MKDEV(MSR_MAJOR, cpu), NULL,
+			    "msr%d", cpu);
+	return PTR_ERR_OR_ZERO(dev);
+}
+
+static int msr_device_destroy(unsigned int cpu)
+{
+	device_destroy(msr_class, MKDEV(MSR_MAJOR, cpu));
+	return 0;
+}
+
+static char *msr_devnode(struct device *dev, umode_t *mode)
+{
+	return kasprintf(GFP_KERNEL, "cpu/%u/msr", MINOR(dev->devt));
+}
+
+static int __init msr_init(void)
+{
+	int err;
+
+	err = __register_chrdev(MSR_MAJOR, 0, NR_CPUS, "cpu/msr", &msr_fops);
+	if (err < 0) {
+		pr_err("unable to get major %d for msr\n", MSR_MAJOR);
+		return -EBUSY;
+	}
+
+	msr_class = class_create(THIS_MODULE, "msr");
+	if (IS_ERR(msr_class)) {
+		err = PTR_ERR(msr_class);
+		goto out_chrdev;
+	}
+	msr_class->devnode = msr_devnode;
+
+	/* TODO: set callback function for hotplug */
+	err  = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/msr:online",
+				 msr_device_create, msr_device_destroy);
+	if (err < 0)
+		goto out_class;
+	cpuhp_msr_state = err;
+
+	/*
+	 * register two hooks to block undef instruction exception
+	 * in EL1 and EL2.
+	 */
+	register_undef_hook(&mrs_hook_el1);
+	register_undef_hook(&msr_hook_el1);
+	register_undef_hook(&mrs_hook_el2);
+	register_undef_hook(&msr_hook_el2);
+
+	/*
+	 * Note: it is absolutely necessary to initialize the physical address of
+	 * MSR and MRS instructions.
+	 */
+	err = rdmsr_safe_on_cpu_aarch64(0, 0, NULL, NULL);
+	err = wrmsr_safe_on_cpu_aarch64(0, 0, 0, 0);
+	return 0;
+
+out_class:
+	class_destroy(msr_class);
+out_chrdev:
+	__unregister_chrdev(MSR_MAJOR, 0, NR_CPUS, "cpu/msr");
+	return err;
+}
+module_init(msr_init);
+
+static void __exit msr_exit(void)
+{
+	/* unregister the hook of MSR and MRS instruction. */
+	unregister_undef_hook(&mrs_hook_el1);
+	unregister_undef_hook(&msr_hook_el1);
+	unregister_undef_hook(&mrs_hook_el2);
+	unregister_undef_hook(&msr_hook_el2);
+	cpuhp_remove_state(cpuhp_msr_state);
+	class_destroy(msr_class);
+	__unregister_chrdev(MSR_MAJOR, 0, NR_CPUS, "cpu/msr");
+}
+module_exit(msr_exit)
+
+MODULE_AUTHOR("Rongwei Wang <rongwei.wang@linux.alibaba.com>");
+MODULE_DESCRIPTION("ARM system register driver");
+MODULE_LICENSE("GPL");
diff --git a/arch/arm64/kernel/msr_smp.c b/arch/arm64/kernel/msr_smp.c
new file mode 100644
index 0000000..0ec0a03
--- /dev/null
+++ b/arch/arm64/kernel/msr_smp.c
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MSR ARM Module support.
+ *
+ * Copyright (c) 2020, Alibaba Group.
+ * Authors: Rongwei Wang <rongwei.wang@linux.alibaba.com>
+ */
+#include <linux/export.h>
+#include <linux/preempt.h>
+#include <linux/smp.h>
+#include <linux/completion.h>
+#include <linux/mm.h>
+#include <linux/highmem.h>
+#include <asm/tlbflush.h>
+#include <asm/cacheflush.h>
+#include <asm/pgtable.h>
+#include <linux/uaccess.h>
+#include <linux/pagemap.h>
+#include <asm/msr_arm.h>
+
+static DEFINE_RAW_SPINLOCK(msr_lock);
+/* record the address of reading or writing */
+static u32 *rd_tp;
+static u32 *wr_tp;
+
+atomic_t msr_flags = ATOMIC_INIT(0);
+EXPORT_SYMBOL(msr_flags);
+atomic_t mrs_flags = ATOMIC_INIT(0);
+EXPORT_SYMBOL(mrs_flags);
+
+struct msr_info_completion {
+	struct msr_info		msr;
+	struct completion	done;
+};
+
+/*
+ * Self-modify code for label of read address.
+ */
+int aarch64_modify_read_text(u32 opcode)
+{
+	void *addrs[1];
+
+	addrs[0] = rd_tp;
+	/*
+	 * call aarch64_insn_patch_text to modify
+	 * the opcode
+	 */
+	return aarch64_insn_patch_text(addrs, &opcode, 1);
+}
+EXPORT_SYMBOL(aarch64_modify_read_text);
+
+/*
+ * Self-modify code for label of write address.
+ */
+int aarch64_modify_write_text(u32 opcode)
+{
+	void *addrs[1];
+
+	addrs[0] = wr_tp;
+	/*
+	 * call aarch64_insn_patch_text to modify
+	 * the opcode
+	 */
+	return aarch64_insn_patch_text(addrs, &opcode, AARCH64_INSN_SIZE);
+}
+EXPORT_SYMBOL(aarch64_modify_write_text);
+
+/*
+ * return a address of read or write label
+ */
+u32 *get_read_insn_addr(void)
+{
+	/*
+	 * TODO: rd_tp on each cpu
+	 * This is a vmalloc address for module.
+	 */
+	return rd_tp;
+}
+EXPORT_SYMBOL(get_read_insn_addr);
+
+u32 *get_write_insn_addr(void)
+{
+	/* This is a vmalloc address. */
+	return wr_tp;
+}
+EXPORT_SYMBOL(get_write_insn_addr);
+
+/*
+ * Read data from register
+ *
+ * At runtime, the "mrs xyz, xyz" instruction will be modified through rd_tp
+ * address.
+ */
+static noinline int rdmsr_safe_aarch64(u32 opt, u32 *data0, u32 *data1)
+{
+	/* reg is encoded by op0,op1,cn... */
+	u32 err = 0;
+	unsigned long __val = 0;
+	unsigned long __pc_addr = 0;
+
+	if ((data0 == NULL) && (data1 == NULL)) {
+		/* init read or write address in somewhere */
+		return 0;
+	}
+
+	raw_spin_lock(&msr_lock);
+	atomic_add(1, &mrs_flags);
+	/*
+	 * On the first execution, opt=0, will NOT execute "mrs xyz, xyz"
+	 * instruction and ONLY initializes rd_tp value.
+	 * In addition, "mrs xyz, xyz" instruction will be modified before running.
+	 */
+	asm volatile("mov %3, 0\n\t"
+			"cmp %4, 0\n\t"
+			"b.eq 1f\n\t"
+			"mrs %0, MIDR_EL1\n\t"
+			"b 1f\n\t"
+			"mov %3, 1\n\t" /* Execute only when an exception occurs */
+			"1:adr %1, .\n\t"
+			"sub %1, %1, 12\n\t"
+			"mov %2, %1\n\t"
+			: "=r"(__val), "=r"(__pc_addr), "=r"(rd_tp), "=&r"(err)
+			: "r"(opt));
+	atomic_sub(1, &mrs_flags);
+	raw_spin_unlock(&msr_lock);
+	if (err == 1) {
+		/* undef instruction occurred */
+		return -1;
+	}
+
+	*data0 = __val;
+	*data1 = __val >> 32;
+	/* be successful */
+	return 0;
+}
+
+/*
+ * Write data to register
+ *
+ * At runtime, the "msr xyz, xyz" instruction will be modified through wr_tp
+ * address.
+ */
+static noinline int wrmsr_safe_aarch64(u32 opt, u32 data0, u32 data1)
+{
+	unsigned long __val = 0;
+	unsigned long __pc_addr = 0;
+	u64 data = 0;
+	int err = 0;
+
+	data = data1;
+	data = (data << 32) | (data0);
+	__val = data;
+	/*
+	 * Add a lock to the following code, and a flag (msr_flags) that the
+	 * current MSR is executing. This flag variable will work in hooker
+	 * function.
+	 */
+	raw_spin_lock(&msr_lock);
+	atomic_add(1, &msr_flags);
+	/*
+	 * On the first execution, opt=0, will NOT execute "msr xyz, xyz"
+	 * instruction and ONLY initializes wr_tp value.
+	 * In addition, "msr xyz, xyz" instruction will be modified before running.
+	 */
+	asm volatile("mov %2, 0\n\t"
+			"cmp %4, 0\n\t"
+			"b.eq 1f\n\t"
+			"msr TCR_EL1, %3\n\t"
+			"b 1f\n\t"
+			"mov %2, 1\n\t" /* exec when exception occurred */
+			"1:adr %0, .\n\t"
+			"sub %0, %0, 12\n\t"
+			"mov %1, %0\n\t"
+			: "=r"(__pc_addr), "=r"(wr_tp), "=&r"(err)
+			: "rZ"(__val), "r"(opt));
+	atomic_sub(1, &msr_flags);
+	raw_spin_unlock(&msr_lock);
+	if (err == 1) {
+		/* undef instruction occurred */
+		return -1;
+	}
+	/* be successful */
+	return 0;
+}
+
+/*
+ * These "safe" variants are slower and should be used when the target MSR
+ * may not actually exist.
+ */
+static void __rdmsr_safe_on_cpu_aarch64(void *info)
+{
+	struct msr_info_completion *rv = info;
+
+	rv->msr.err = rdmsr_safe_aarch64(rv->msr.opt, &rv->msr.reg.l,
+			&rv->msr.reg.h);
+	complete(&rv->done);
+}
+
+static void __wrmsr_safe_on_cpu_aarch64(void *info)
+{
+	struct msr_info *rv = info;
+
+	rv->err = wrmsr_safe_aarch64(rv->opt, rv->reg.l, rv->reg.h);
+}
+
+int rdmsr_safe_on_cpu_aarch64(u32 cpu, u32 opt, u32 *l, u32 *h)
+{
+	struct msr_info_completion rv;
+	call_single_data_t csd = {
+		.func	= __rdmsr_safe_on_cpu_aarch64,
+		.info	= &rv,
+	};
+	int err;
+
+	memset(&rv, 0, sizeof(rv));
+	init_completion(&rv.done);
+	rv.msr.opt = opt;
+
+	err = smp_call_function_single_async(cpu, &csd);
+	if (!err) {
+		wait_for_completion_timeout(&rv.done, msecs_to_jiffies(5000));
+		err = rv.msr.err;
+	}
+
+	if ((l != NULL) && (h != NULL)) {
+		*l = rv.msr.reg.l;
+		*h = rv.msr.reg.h;
+	}
+
+	return err;
+}
+EXPORT_SYMBOL(rdmsr_safe_on_cpu_aarch64);
+
+int wrmsr_safe_on_cpu_aarch64(u32 cpu, u32 opt, u32 l, u32 h)
+{
+	int err;
+	struct msr_info rv;
+
+	memset(&rv, 0, sizeof(rv));
+
+	rv.opt = opt;
+	rv.reg.l = l;
+	rv.reg.h = h;
+	err = smp_call_function_single(cpu,
+			__wrmsr_safe_on_cpu_aarch64, &rv, 1);
+
+	return err ? err : rv.err;
+}
+EXPORT_SYMBOL(wrmsr_safe_on_cpu_aarch64);
-- 
1.8.3.1

[PATCH 3/3] arm64:msr: Enable MSR ARM driver

[Rongwei Wang]

By default, MSR-ARM is compiled as a module.

Signed-off-by: Rongwei Wang <rongwei.wang@linux.alibaba.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 5cfe3cf..952cf6a 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1092,3 +1092,4 @@ CONFIG_DEBUG_KERNEL=y
 # CONFIG_DEBUG_PREEMPT is not set
 # CONFIG_FTRACE is not set
 CONFIG_MEMTEST=y
+CONFIG_ARM64_MSR=m
-- 
1.8.3.1

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Will Deacon]

On Tue, Dec 01, 2020 at 01:48:30AM +0800, Rongwei Wang wrote:
> MSR ARM driver aims to provide interfacs for user to read or write data to all
> system registers. Its functions is same as MSR driver (x86 platform). It mainly
> depends on kprobe and undef exception to read or write system registers
> dynamicly.

... but why? These are privileged registers for a reason, and giving
userspace access to them is bound to cause serious problems. Why can't we
do whatever it is that userspace is trying to do by poking these things in
the kernel instead? This interface is a bit like using /dev/mem instead of
writing a proper device driver.

Thanks,

Will

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Marc Zyngier]

On 2020-11-30 17:48, Rongwei Wang wrote:

> The interfaces of this module is same as MSR module in user space, and 
> to solve
> the problem that ARM platform has no similar MSR module. Using this 
> interface,
> we did some pressure tests to test the stability and security of MSR 
> driver. The
> test results show that the driver will not cause system panic if 
> various
> illegal values and multithreading concurrent access are passed through 
> the
> interface.

It would certainly help if you described what problem you are trying
to solve here. In general, giving userspace access to random system
registers is a pretty bad idea.

Are you trying to validate a CPU? a hypervisor? Or is it just a fun way
to check how many things you can poke before something catches fire?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Randy Dunlap]

On 11/30/20 10:05 AM, Marc Zyngier wrote:
> On 2020-11-30 17:48, Rongwei Wang wrote:
> 
>> The interfaces of this module is same as MSR module in user space, and to solve
>> the problem that ARM platform has no similar MSR module. Using this interface,
>> we did some pressure tests to test the stability and security of MSR driver. The
>> test results show that the driver will not cause system panic if various
>> illegal values and multithreading concurrent access are passed through the
>> interface.
> 
> It would certainly help if you described what problem you are trying
> to solve here. In general, giving userspace access to random system
> registers is a pretty bad idea.
> 
> Are you trying to validate a CPU? a hypervisor? Or is it just a fun way
> to check how many things you can poke before something catches fire?

I agree with the requests for justification.

Rongwei mentions that this driver functions the same as the
x86 platform MSR driver.  This one uses /dev.
I thought that the x86 driver used /sys, but I could be wrong.

Do we (or not) want cross-platform compatabilities?

Regarding permissions, /dev or /sys files have permission settings.
They don't have to be world-readable.


-- 
~Randy

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Borislav Petkov]

On Tue, Dec 01, 2020 at 01:48:30AM +0800, Rongwei Wang wrote:
> MSR ARM driver aims to provide interfacs for user to read or write
> data to all system registers.

Just a warranty from x86 land: if I were an ARM arch maintainer, I would
never never *ever* take such driver exposing naked hw registers to
userspace.

We have been fighting with this on x86 for years:

a7e1f67ed29f ("x86/msr: Filter MSR writes")

with userspace tools poking at random MSRs. Read the commit message
for what can happen. And taking that thing is like opening a huge
can'o'worms that can't be closed anymore.

Currently, we're trying to move userspace tools to proper sysfs
interfaces but it is a huuuge pain. It is a lot easier to have people
define proper interfaces from the get-go where the kernel can control
and synchronize access.

HTH.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[wangrongwei]

Hi, Will
Thanks for your reply!
There are absolutely privileged registers, and seriously expose the interface directly! 
However, these interfaces (/dev/cpu/<N>/msr) need permissions to access, so there should be few security issues. In addition, it may be due to my unclear description above. This driver is mainly a debugging tool. It make developers can easily get the values of some system registers without modifying the kernel.
In fact, we first considered the /dev/mem solution, but it seems that /dev/mem can map peripheral or IO registers, but not system registers.

> 2020年12月1日 上午1:57，Will Deacon <will@kernel.org> 写道：
> 
> On Tue, Dec 01, 2020 at 01:48:30AM +0800, Rongwei Wang wrote:
>> MSR ARM driver aims to provide interfacs for user to read or write data to all
>> system registers. Its functions is same as MSR driver (x86 platform). It mainly
>> depends on kprobe and undef exception to read or write system registers
>> dynamicly.
> 
> ... but why? These are privileged registers for a reason, and giving
> userspace access to them is bound to cause serious problems. Why can't we
> do whatever it is that userspace is trying to do by poking these things in
> the kernel instead? This interface is a bit like using /dev/mem instead of
> writing a proper device driver.
> 
> Thanks,
> 
> Will

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[wangrongwei]

Hi
We have validate this driver in vm and physical machine, and works fine.
Actually, we used existing interfaces to realize this driver, likes aarch64_insn_read and aarch64_insn_patch_text. These existing intefaces had validated a CPU. 

> 2020年12月1日 上午2:05，Marc Zyngier <maz@kernel.org> 写道：
> 
> On 2020-11-30 17:48, Rongwei Wang wrote:
> 
>> The interfaces of this module is same as MSR module in user space, and to solve
>> the problem that ARM platform has no similar MSR module. Using this interface,
>> we did some pressure tests to test the stability and security of MSR driver. The
>> test results show that the driver will not cause system panic if various
>> illegal values and multithreading concurrent access are passed through the
>> interface.
> 
> It would certainly help if you described what problem you are trying
> to solve here. In general, giving userspace access to random system
> registers is a pretty bad idea.
> 
> Are you trying to validate a CPU? a hypervisor? Or is it just a fun way
> to check how many things you can poke before something catches fire?
> 
> Thanks,
> 
>        M.
> -- 
> Jazz is not dead. It just smells funny...

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[wangrongwei]

Hi, Boris, Thank your advice very much!
Indeed, I have read the commit message, and it seems that writes data to a system register may cause many problems. Actually, we have taken this into account. In the current version, we have separated the read and write functions to the system registers into two commands, they (rdasr and wrasr) can be found in system-register-tools (https://github.com/alibaba/system-register-tools).

In providing the WRASR function, we consider that users should bear the consequences of rewriting registers during the debugging phase. In fact, most of the time we rarely use WRASR, and only use it when we are very confident.

> 2020年12月1日 上午3:03，Borislav Petkov <bp@alien8.de> 写道：
> 
> On Tue, Dec 01, 2020 at 01:48:30AM +0800, Rongwei Wang wrote:
>> MSR ARM driver aims to provide interfacs for user to read or write
>> data to all system registers.
> 
> Just a warranty from x86 land: if I were an ARM arch maintainer, I would
> never never *ever* take such driver exposing naked hw registers to
> userspace.
> 
> We have been fighting with this on x86 for years:
> 
> a7e1f67ed29f ("x86/msr: Filter MSR writes")
> 
> with userspace tools poking at random MSRs. Read the commit message
> for what can happen. And taking that thing is like opening a huge
> can'o'worms that can't be closed anymore.
> 
> Currently, we're trying to move userspace tools to proper sysfs
> interfaces but it is a huuuge pain. It is a lot easier to have people
> define proper interfaces from the get-go where the kernel can control
> and synchronize access.
> 
> HTH.
> 
> -- 
> Regards/Gruss,
>    Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[wangrongwei]

Hi, Will
Thanks for your reply!
There are absolutely privileged registers, and seriously expose the interface directly! 
However, these interfaces (/dev/cpu/<N>/msr) need permissions to access, so there should be few security issues. In addition, it may be due to my unclear description above. This driver is mainly a debugging tool. It make developers can easily get the values of some system registers without modifying the kernel.
In fact, we first considered the /dev/mem solution, but it seems that /dev/mem can map peripheral or IO registers, but not system registers.

> 2020年12月1日 上午1:57，Will Deacon <will@kernel.org> 写道：
> 
> On Tue, Dec 01, 2020 at 01:48:30AM +0800, Rongwei Wang wrote:
>> MSR ARM driver aims to provide interfacs for user to read or write data to all
>> system registers. Its functions is same as MSR driver (x86 platform). It mainly
>> depends on kprobe and undef exception to read or write system registers
>> dynamicly.
> 
> ... but why? These are privileged registers for a reason, and giving
> userspace access to them is bound to cause serious problems. Why can't we
> do whatever it is that userspace is trying to do by poking these things in
> the kernel instead? This interface is a bit like using /dev/mem instead of
> writing a proper device driver.
> 
> Thanks,
> 
> Will

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Marc Zyngier]

On 2020-12-01 03:09, wangrongwei wrote:
> Hi
> We have validate this driver in vm and physical machine, and works 
> fine.

But what does "work fine" mean? None of these system registers are 
supposed
to be accessible from userspace, so please explain *what* you are trying 
to
do with this, other that introducing security holes and general system
instability?

> Actually, we used existing interfaces to realize this driver, likes
> aarch64_insn_read and aarch64_insn_patch_text.

Sure. At that stage, you could also directly expose the linear mapping 
to
userspace and start writing to it, it would probably be a fitting 
addition...

> These existing intefaces had validated a CPU.

If CPU validation is your goal, I suggest this is kept out of tree, as 
the
kernel is hardly a validation tool for the arm64 architecture.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Borislav Petkov]

On Tue, Dec 01, 2020 at 11:44:52AM +0800, wangrongwei wrote:
> Indeed, I have read the commit message, and it seems that writes data
> to a system register may cause many problems. Actually, we have taken
> this into account. In the current version, we have separated the read
> and write functions to the system registers into two commands,

There's rdmsr and wrmsr in msr-tools on x86 too.

> In providing the WRASR function, we consider that users should bear
> the consequences of rewriting registers during the debugging phase. In
> fact, most of the time we rarely use WRASR, and only use it when we
> are very confident.

As I said, there should never never ever be a tool which allows writing
of registers from userspace. If I could go back in time, I'd stop this
on x86 but it is too late now. Not for ARM though.

Also, do you see how all the people who replied to your mail, put the
text under the quoted text. Do that too, pls, from now on, and refrain
from top-posting.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[wangrongwei]

> 2020年12月1日 下午4:12，Marc Zyngier <maz@kernel.org> 写道：
> 
> On 2020-12-01 03:09, wangrongwei wrote:
>> Hi
>> We have validate this driver in vm and physical machine, and works fine.
> 
> But what does "work fine" mean? None of these system registers are supposed
> to be accessible from userspace, so please explain *what* you are trying to
> do with this, other that introducing security holes and general system
> instability?
I think I know what you mean. Do you want me to describe how we achieved it?
In x86, the different registers can be accessed directly using the rdmsr and wrmsr instructions, but in ARM, since these two instructions are missing, so we modify the code segment during runtime, similar to the principle of static_key.
> 
>> Actually, we used existing interfaces to realize this driver, likes
>> aarch64_insn_read and aarch64_insn_patch_text.
> 
> Sure. At that stage, you could also directly expose the linear mapping to
> userspace and start writing to it, it would probably be a fitting addition...
Yes, but accessing those system registers doesn't seem to have an address, just have a register number (I'm not sure about that)?
> 
>> These existing intefaces had validated a CPU.
> 
> If CPU validation is your goal, I suggest this is kept out of tree, as the
> kernel is hardly a validation tool for the arm64 architecture.
Not really sure what you mean by CPU validation as you describe above? However, the biggest challenge we encountered during implementation was an undefined instruction exception that could occur after modifying a code segment, but this problem was also solved using the kernel's existing interface.
> 
> Thanks,
> 
>        M.
> -- 
> Jazz is not dead. It just smells funny…
Regard,
Rongwei.

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[wangrongwei]

> 2020年12月1日 下午7:26，Borislav Petkov <bp@alien8.de> 写道：
> 
> On Tue, Dec 01, 2020 at 11:44:52AM +0800, wangrongwei wrote:
>> Indeed, I have read the commit message, and it seems that writes data
>> to a system register may cause many problems. Actually, we have taken
>> this into account. In the current version, we have separated the read
>> and write functions to the system registers into two commands,
> 
> There's rdmsr and wrmsr in msr-tools on x86 too.
Yes, and x86 also provides two instructions with the same name in the instruction set, but not in ARM.
> 
>> In providing the WRASR function, we consider that users should bear
>> the consequences of rewriting registers during the debugging phase. In
>> fact, most of the time we rarely use WRASR, and only use it when we
>> are very confident.
> 
> As I said, there should never never ever be a tool which allows writing
> of registers from userspace. If I could go back in time, I'd stop this
> on x86 but it is too late now. Not for ARM though.
> 
> Also, do you see how all the people who replied to your mail, put the
> text under the quoted text. Do that too, pls, from now on, and refrain
> from top-posting.
Thank you for the reminder, it has been corrected!
> 
> Thx.
> 
> -- 
> Regards/Gruss,
>    Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
Thanks,
Rongwei Wang.

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Borislav Petkov]

On Tue, Dec 01, 2020 at 10:33:42PM +0800, wangrongwei wrote:
> Yes, and x86 also provides two instructions with the same name in the
> instruction set, but not in ARM.

Sorry, I can't parse what you're trying to say here.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Rongwei Wang]

> 2020年12月1日 下午10:54，Borislav Petkov <bp@alien8.de> 写道：
> 
> On Tue, Dec 01, 2020 at 10:33:42PM +0800, wangrongwei wrote:
>> Yes, and x86 also provides two instructions with the same name in the
>> instruction set, but not in ARM.
> 
> Sorry, I can't parse what you're trying to say here.
In the ARM architecture, there are no rdmsr and wrmsr instructions. 
In ARM, the system registers can only be accessed through msr and mrs, so the problem created by MSR driver (depend on rdmsr and wrmsr) in x86 is not necessarily present in ARM, which is very different from x86.
In addition, we want cross-platform compatabilities, and fill these gaps in the ARM.
> 
> -- 
> Regards/Gruss,
>    Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
Regards,
Rongwei Wang.

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Borislav Petkov]

On Tue, Dec 01, 2020 at 11:17:39PM +0800, Rongwei Wang wrote:
> In ARM, the system registers can only be accessed through msr and mrs,
> so the problem created by MSR driver (depend on rdmsr and wrmsr) in
> x86 is not necessarily present in ARM, which is very different from
> x86.

No, the point I'm making is that it doesn't matter what the architecture
does or does not, register *writes* from userspace are a bad bad idea
for the reasons I described.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Ard Biesheuvel]

On Tue, 1 Dec 2020 at 16:17, Rongwei Wang
<rongwei.wang@linux.alibaba.com> wrote:
>
>
>
> > 2020年12月1日 下午10:54，Borislav Petkov <bp@alien8.de> 写道：
> >
> > On Tue, Dec 01, 2020 at 10:33:42PM +0800, wangrongwei wrote:
> >> Yes, and x86 also provides two instructions with the same name in the
> >> instruction set, but not in ARM.
> >
> > Sorry, I can't parse what you're trying to say here.
> In the ARM architecture, there are no rdmsr and wrmsr instructions.
> In ARM, the system registers can only be accessed through msr and mrs, so the problem created by MSR driver (depend on rdmsr and wrmsr) in x86 is not necessarily present in ARM, which is very different from x86.
> In addition, we want cross-platform compatabilities, and fill these gaps in the ARM.

Apologies for being blunt, but this looks like another occurrence of
the 'compatibility' disease, where everything has to work in the exact
same way as it does on x86, even if it was a terrible idea to begin
with.

One of the nice things of having a [relatively] new architecture is
that we are not being held back by backward compatibility
requirements, given that we cannot break existing use cases that
predate the existence of the architecture.

In other words, there are things that the x86 maintainers would gladly
remove (such as MSR access from userland, and I'm sure there are many
examples), but they simply cannot because it is already being relied
upon in the field. But that does not mean other architectures are
required to provide a similar interface, and especially not ones that
allow you to poke at undocumented, non-architected CPU registers.

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Marc Zyngier]

On 2020-12-01 14:25, wangrongwei wrote:
>> 2020年12月1日 下午4:12，Marc Zyngier <maz@kernel.org> 写道：
>> 
>> On 2020-12-01 03:09, wangrongwei wrote:
>>> Hi
>>> We have validate this driver in vm and physical machine, and works 
>>> fine.
>> 
>> But what does "work fine" mean? None of these system registers are 
>> supposed
>> to be accessible from userspace, so please explain *what* you are 
>> trying to
>> do with this, other that introducing security holes and general system
>> instability?
> I think I know what you mean. Do you want me to describe how we 
> achieved it?
> In x86, the different registers can be accessed directly using the
> rdmsr and wrmsr instructions, but in ARM, since these two instructions
> are missing, so we modify the code segment during runtime, similar to
> the principle of static_key.

[...]

These are implementation details, none of which answer my question:

What makes you think this is a good idea? I cannot see any legitimate
reason for userspace to ever access privileged system registers, and
the fact that x86 has such feature isn't a good justification.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Rongwei Wang]

> 2020年12月1日 下午11:25，Borislav Petkov <bp@alien8.de> 写道：
> 
> On Tue, Dec 01, 2020 at 11:17:39PM +0800, Rongwei Wang wrote:
>> In ARM, the system registers can only be accessed through msr and mrs,
>> so the problem created by MSR driver (depend on rdmsr and wrmsr) in
>> x86 is not necessarily present in ARM, which is very different from
>> x86.
> 
> No, the point I'm making is that it doesn't matter what the architecture
> does or does not, register *writes* from userspace are a bad bad idea
> for the reasons I described.
Sorry, I was away for two days and didn't reply in time.
In fact, we find MSR in the x86 architecture to be a particularly useful interface. It helped us a lot when we were doing some kernel debugging. So we consider implementing a similar feature in ARM.
So as a debugging tool, could it exist, e.g. if I change CONFIG_ARM64_MSR to CONFIG_ARM64_MSR_DEBUG or others. Do you think this adjustment is possible?
> 
> -- 
> Regards/Gruss,
>    Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Regards,
Rongwei Wang.

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Rongwei Wang]

> 2020年12月1日 下午11:37，Marc Zyngier <maz@kernel.org> 写道：
> 
> On 2020-12-01 14:25, wangrongwei wrote:
>>> 2020年12月1日 下午4:12，Marc Zyngier <maz@kernel.org> 写道：
>>> On 2020-12-01 03:09, wangrongwei wrote:
>>>> Hi
>>>> We have validate this driver in vm and physical machine, and works fine.
>>> But what does "work fine" mean? None of these system registers are supposed
>>> to be accessible from userspace, so please explain *what* you are trying to
>>> do with this, other that introducing security holes and general system
>>> instability?
>> I think I know what you mean. Do you want me to describe how we achieved it?
>> In x86, the different registers can be accessed directly using the
>> rdmsr and wrmsr instructions, but in ARM, since these two instructions
>> are missing, so we modify the code segment during runtime, similar to
>> the principle of static_key.
> 
> [...]
> 
> These are implementation details, none of which answer my question:
> 
> What makes you think this is a good idea? I cannot see any legitimate
In fact, I think this tool useful mainly in the following scenarios:
	1. performance debug
	2. Arm-core features test
	3. Debug-tool for kernel developer 
Also, for example, MSR-ARM is needed for chip verification and system-level functional verification.
A simple example, perf stat can test pmu, but the overflow interrupt function and forced overflow function of pmu is not covered.
In both cases, we need a special interface to configure it, which can be considered as testing requirements, so it can only be tested by configuring (access) registers, e.g., devmem command for memmap registers, MSR-ARM driver for system registers.
> reason for userspace to ever access privileged system registers, and
> the fact that x86 has such feature isn't a good justification.
> 
>        M.
> -- 
> Jazz is not dead. It just smells funny…

Sorry for the delayed response.

Regards,
Rongwei Wang.

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Marc Zyngier]

On 2020-12-03 05:45, Rongwei Wang wrote:
>> 2020年12月1日 下午11:37，Marc Zyngier <maz@kernel.org> 写道：
>> 
>> On 2020-12-01 14:25, wangrongwei wrote:
>>>> 2020年12月1日 下午4:12，Marc Zyngier <maz@kernel.org> 写道：
>>>> On 2020-12-01 03:09, wangrongwei wrote:
>>>>> Hi
>>>>> We have validate this driver in vm and physical machine, and works 
>>>>> fine.
>>>> But what does "work fine" mean? None of these system registers are 
>>>> supposed
>>>> to be accessible from userspace, so please explain *what* you are 
>>>> trying to
>>>> do with this, other that introducing security holes and general 
>>>> system
>>>> instability?
>>> I think I know what you mean. Do you want me to describe how we 
>>> achieved it?
>>> In x86, the different registers can be accessed directly using the
>>> rdmsr and wrmsr instructions, but in ARM, since these two 
>>> instructions
>>> are missing, so we modify the code segment during runtime, similar to
>>> the principle of static_key.
>> 
>> [...]
>> 
>> These are implementation details, none of which answer my question:
>> 
>> What makes you think this is a good idea? I cannot see any legitimate
> In fact, I think this tool useful mainly in the following scenarios:
> 	1. performance debug
> 	2. Arm-core features test
> 	3. Debug-tool for kernel developer
> Also, for example, MSR-ARM is needed for chip verification and
> system-level functional verification.
> A simple example, perf stat can test pmu, but the overflow interrupt
> function and forced overflow function of pmu is not covered.

But what does it mean to change random system registers while the kernel
itself is using them in parallel? All you are introducing is a bunch of
uncontrolled, unexpected, and possibly fatal side effects.

Introducing such an interface makes the kernel unsafe, insecure, and
and violates all the possible guarantees that we are trying hard to
provide. After all, why would we even try to mitigate side channel
vulnerabilities if we were to introduce such a thing?

> In both cases, we need a special interface to configure it, which can
> be considered as testing requirements, so it can only be tested by
> configuring (access) registers, e.g., devmem command for memmap
> registers, MSR-ARM driver for system registers.

devmem was a terrible mistake. Unprivileged sysreg access is another
instance of the same mistake.

The kernel is not a validation tool. It is designed to operate safely,
securely, and reliably. What you propose is the negation of these goals
for dubious purposes, and I think I represent a large number of kernel
developers when I say that we really do not want it.

This will (hopefully) be my last message on this subject.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Rongwei Wang]

> 2020年12月3日 下午4:35，Marc Zyngier <maz@kernel.org> 写道：
> 
> On 2020-12-03 05:45, Rongwei Wang wrote:
>>> 2020年12月1日 下午11:37，Marc Zyngier <maz@kernel.org> 写道：
>>> On 2020-12-01 14:25, wangrongwei wrote:
>>>>> 2020年12月1日 下午4:12，Marc Zyngier <maz@kernel.org> 写道：
>>>>> On 2020-12-01 03:09, wangrongwei wrote:
>>>>>> Hi
>>>>>> We have validate this driver in vm and physical machine, and works fine.
>>>>> But what does "work fine" mean? None of these system registers are supposed
>>>>> to be accessible from userspace, so please explain *what* you are trying to
>>>>> do with this, other that introducing security holes and general system
>>>>> instability?
>>>> I think I know what you mean. Do you want me to describe how we achieved it?
>>>> In x86, the different registers can be accessed directly using the
>>>> rdmsr and wrmsr instructions, but in ARM, since these two instructions
>>>> are missing, so we modify the code segment during runtime, similar to
>>>> the principle of static_key.
>>> [...]
>>> These are implementation details, none of which answer my question:
>>> What makes you think this is a good idea? I cannot see any legitimate
>> In fact, I think this tool useful mainly in the following scenarios:
>> 	1. performance debug
>> 	2. Arm-core features test
>> 	3. Debug-tool for kernel developer
>> Also, for example, MSR-ARM is needed for chip verification and
>> system-level functional verification.
>> A simple example, perf stat can test pmu, but the overflow interrupt
>> function and forced overflow function of pmu is not covered.
> 
> But what does it mean to change random system registers while the kernel
> itself is using them in parallel? All you are introducing is a bunch of
> uncontrolled, unexpected, and possibly fatal side effects.
This problem exists when writing to a register, but it does not exist when reading a register.
> 
> Introducing such an interface makes the kernel unsafe, insecure, and
> and violates all the possible guarantees that we are trying hard to
> provide. After all, why would we even try to mitigate side channel
> vulnerabilities if we were to introduce such a thing?
> 
>> In both cases, we need a special interface to configure it, which can
>> be considered as testing requirements, so it can only be tested by
>> configuring (access) registers, e.g., devmem command for memmap
>> registers, MSR-ARM driver for system registers.
> 
> devmem was a terrible mistake. Unprivileged sysreg access is another
> instance of the same mistake.
> 
> The kernel is not a validation tool. It is designed to operate safely,
> securely, and reliably. What you propose is the negation of these goals
> for dubious purposes, and I think I represent a large number of kernel
> developers when I say that we really do not want it.
> 
> This will (hopefully) be my last message on this subject.
> 
> Thanks,
> 
>        M.
> -- 
> Jazz is not dead. It just smells funny…
Thanks,
Rongwei Wang.

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Marc Zyngier]

On 2020-12-03 11:25, Rongwei Wang wrote:
>> 2020年12月3日 下午4:35，Marc Zyngier <maz@kernel.org> 写道：

[...]

>> But what does it mean to change random system registers while the 
>> kernel
>> itself is using them in parallel? All you are introducing is a bunch 
>> of
>> uncontrolled, unexpected, and possibly fatal side effects.
> This problem exists when writing to a register, but it does not exist
> when reading a register.

If you're not aware that the ARM architecture does have system registers
with read side-effects, you really shouldn't be writing this code.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Mark Rutland]

On Thu, Dec 03, 2020 at 07:25:43PM +0800, Rongwei Wang wrote:
> 
> 
> > 2020年12月3日 下午4:35，Marc Zyngier <maz@kernel.org> 写道：
> > 
> > On 2020-12-03 05:45, Rongwei Wang wrote:
> >>> 2020年12月1日 下午11:37，Marc Zyngier <maz@kernel.org> 写道：
> >>> On 2020-12-01 14:25, wangrongwei wrote:
> >>>>> 2020年12月1日 下午4:12，Marc Zyngier <maz@kernel.org> 写道：
> >>>>> On 2020-12-01 03:09, wangrongwei wrote:
> >>>>>> Hi
> >>>>>> We have validate this driver in vm and physical machine, and works fine.
> >>>>> But what does "work fine" mean? None of these system registers are supposed
> >>>>> to be accessible from userspace, so please explain *what* you are trying to
> >>>>> do with this, other that introducing security holes and general system
> >>>>> instability?
> >>>> I think I know what you mean. Do you want me to describe how we achieved it?
> >>>> In x86, the different registers can be accessed directly using the
> >>>> rdmsr and wrmsr instructions, but in ARM, since these two instructions
> >>>> are missing, so we modify the code segment during runtime, similar to
> >>>> the principle of static_key.
> >>> [...]
> >>> These are implementation details, none of which answer my question:
> >>> What makes you think this is a good idea? I cannot see any legitimate
> >> In fact, I think this tool useful mainly in the following scenarios:
> >> 	1. performance debug
> >> 	2. Arm-core features test
> >> 	3. Debug-tool for kernel developer
> >> Also, for example, MSR-ARM is needed for chip verification and
> >> system-level functional verification.
> >> A simple example, perf stat can test pmu, but the overflow interrupt
> >> function and forced overflow function of pmu is not covered.
> > 
> > But what does it mean to change random system registers while the kernel
> > itself is using them in parallel? All you are introducing is a bunch of
> > uncontrolled, unexpected, and possibly fatal side effects.
> This problem exists when writing to a register, but it does not exist when reading a register.

Reading registers can also have side-effects. For example the ICC_IAR_*
registers are read-sensitive, and reading those could cause the kernel
to stop receiving timer interrupts or similar. The problem /does/ exist
when reading registers.

I concur with Marc and others that this simply isn't safe, regardless of
read or write, and not something we want upstream in Linux.

For debug and testing, I suggest looking at kvm-unit-tests. That has
some PMU tests already, and is gaining the ability to run bare-metal
around now (and so might be helpful for testing HW). Having improved
tests there would benefit everyone.

Thanks,
Mark.

Re: [PATCH 0/3] arm64:msr: Add MSR driver

[Rongwei Wang]

> On Dec 3, 2020, at 7:45 PM, Marc Zyngier <maz@kernel.org> wrote:
> 
> On 2020-12-03 11:25, Rongwei Wang wrote:
>>> 2020年12月3日 下午4:35，Marc Zyngier <maz@kernel.org> 写道：
> 
> [...]
> 
>>> But what does it mean to change random system registers while the kernel
>>> itself is using them in parallel? All you are introducing is a bunch of
>>> uncontrolled, unexpected, and possibly fatal side effects.
>> This problem exists when writing to a register, but it does not exist
>> when reading a register.
> 
> If you're not aware that the ARM architecture does have system registers
> with read side-effects, you really shouldn't be writing this code.
Thanks, Does it make sense to put this feature in the drivers/ directory, likes misc drivers? Whether it can exist as a debugging tool in kernel or not?
When debugging, then enables CONFIG_ARM64_MSR or CONFIG_ARM64_MSR_DEBUG.
> 
>        M.
> -- 
> Jazz is not dead. It just smells funny…
Thanks,
Rongwei Wang.