* [PATCH 0/3] arm64:msr: Add MSR driver @ 2020-11-30 17:48 Rongwei Wang 2020-11-30 17:48 ` [PATCH 1/3] arm64:insn: Export symbols for MSR ARM driver Rongwei Wang ` (5 more replies) 0 siblings, 6 replies; 28+ messages in thread From: Rongwei Wang @ 2020-11-30 17:48 UTC (permalink / raw) To: catalin.marinas, will, bjorn.andersson, shawnguo Cc: vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/3] arm64:insn: Export symbols for MSR ARM driver 2020-11-30 17:48 [PATCH 0/3] arm64:msr: Add MSR driver Rongwei Wang @ 2020-11-30 17:48 ` Rongwei Wang 2020-11-30 17:48 ` [PATCH 2/3] arm64:msr: Introduce " Rongwei Wang ` (4 subsequent siblings) 5 siblings, 0 replies; 28+ messages in thread From: Rongwei Wang @ 2020-11-30 17:48 UTC (permalink / raw) To: catalin.marinas, will, bjorn.andersson, shawnguo Cc: vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/3] arm64:msr: Introduce MSR ARM driver 2020-11-30 17:48 [PATCH 0/3] arm64:msr: Add MSR driver Rongwei Wang 2020-11-30 17:48 ` [PATCH 1/3] arm64:insn: Export symbols for MSR ARM driver Rongwei Wang @ 2020-11-30 17:48 ` Rongwei Wang 2020-11-30 17:48 ` [PATCH 3/3] arm64:msr: Enable " Rongwei Wang ` (3 subsequent siblings) 5 siblings, 0 replies; 28+ messages in thread From: Rongwei Wang @ 2020-11-30 17:48 UTC (permalink / raw) To: catalin.marinas, will, bjorn.andersson, shawnguo Cc: vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/3] arm64:msr: Enable MSR ARM driver 2020-11-30 17:48 [PATCH 0/3] arm64:msr: Add MSR driver Rongwei Wang 2020-11-30 17:48 ` [PATCH 1/3] arm64:insn: Export symbols for MSR ARM driver Rongwei Wang 2020-11-30 17:48 ` [PATCH 2/3] arm64:msr: Introduce " Rongwei Wang @ 2020-11-30 17:48 ` Rongwei Wang 2020-11-30 17:57 ` [PATCH 0/3] arm64:msr: Add MSR driver Will Deacon ` (2 subsequent siblings) 5 siblings, 0 replies; 28+ messages in thread From: Rongwei Wang @ 2020-11-30 17:48 UTC (permalink / raw) To: catalin.marinas, will, bjorn.andersson, shawnguo Cc: vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-11-30 17:48 [PATCH 0/3] arm64:msr: Add MSR driver Rongwei Wang ` (2 preceding siblings ...) 2020-11-30 17:48 ` [PATCH 3/3] arm64:msr: Enable " Rongwei Wang @ 2020-11-30 17:57 ` Will Deacon 2020-12-01 2:55 ` wangrongwei 2020-12-01 5:44 ` wangrongwei 2020-11-30 18:05 ` Marc Zyngier 2020-11-30 19:03 ` Borislav Petkov 5 siblings, 2 replies; 28+ messages in thread From: Will Deacon @ 2020-11-30 17:57 UTC (permalink / raw) To: Rongwei Wang Cc: catalin.marinas, bjorn.andersson, shawnguo, vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-11-30 17:57 ` [PATCH 0/3] arm64:msr: Add MSR driver Will Deacon @ 2020-12-01 2:55 ` wangrongwei 2020-12-01 5:44 ` wangrongwei 1 sibling, 0 replies; 28+ messages in thread From: wangrongwei @ 2020-12-01 2:55 UTC (permalink / raw) To: Will Deacon Cc: catalin.marinas, bjorn.andersson, shawnguo, vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-11-30 17:57 ` [PATCH 0/3] arm64:msr: Add MSR driver Will Deacon 2020-12-01 2:55 ` wangrongwei @ 2020-12-01 5:44 ` wangrongwei 1 sibling, 0 replies; 28+ messages in thread From: wangrongwei @ 2020-12-01 5:44 UTC (permalink / raw) To: Will Deacon Cc: catalin.marinas, bjorn.andersson, shawnguo, vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-11-30 17:48 [PATCH 0/3] arm64:msr: Add MSR driver Rongwei Wang ` (3 preceding siblings ...) 2020-11-30 17:57 ` [PATCH 0/3] arm64:msr: Add MSR driver Will Deacon @ 2020-11-30 18:05 ` Marc Zyngier 2020-11-30 18:20 ` Randy Dunlap 2020-12-01 3:09 ` wangrongwei 2020-11-30 19:03 ` Borislav Petkov 5 siblings, 2 replies; 28+ messages in thread From: Marc Zyngier @ 2020-11-30 18:05 UTC (permalink / raw) To: Rongwei Wang Cc: catalin.marinas, will, bjorn.andersson, shawnguo, gshan, geert+renesas, Anson.Huang, masahiroy, michael, krzk, linux-kernel, vkoul, olof, vincenzo.frascino, ardb, linux-arm-kernel 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... ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-11-30 18:05 ` Marc Zyngier @ 2020-11-30 18:20 ` Randy Dunlap 2020-12-01 3:09 ` wangrongwei 1 sibling, 0 replies; 28+ messages in thread From: Randy Dunlap @ 2020-11-30 18:20 UTC (permalink / raw) To: Marc Zyngier, Rongwei Wang Cc: catalin.marinas, will, bjorn.andersson, shawnguo, gshan, geert+renesas, Anson.Huang, masahiroy, michael, krzk, linux-kernel, vkoul, olof, vincenzo.frascino, ardb, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-11-30 18:05 ` Marc Zyngier 2020-11-30 18:20 ` Randy Dunlap @ 2020-12-01 3:09 ` wangrongwei 2020-12-01 8:12 ` Marc Zyngier 1 sibling, 1 reply; 28+ messages in thread From: wangrongwei @ 2020-12-01 3:09 UTC (permalink / raw) To: Marc Zyngier Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, gshan, geert+renesas, Anson.Huang, masahiroy, michael, krzk, linux-kernel, vkoul, olof, vincenzo.frascino, ardb, linux-arm-kernel 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... ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-01 3:09 ` wangrongwei @ 2020-12-01 8:12 ` Marc Zyngier 2020-12-01 14:25 ` wangrongwei 0 siblings, 1 reply; 28+ messages in thread From: Marc Zyngier @ 2020-12-01 8:12 UTC (permalink / raw) To: wangrongwei Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, gshan, geert+renesas, Anson.Huang, masahiroy, michael, krzk, linux-kernel, vkoul, olof, vincenzo.frascino, ardb, linux-arm-kernel 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... ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-01 8:12 ` Marc Zyngier @ 2020-12-01 14:25 ` wangrongwei 2020-12-01 15:37 ` Marc Zyngier 0 siblings, 1 reply; 28+ messages in thread From: wangrongwei @ 2020-12-01 14:25 UTC (permalink / raw) To: Marc Zyngier Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, gshan, geert+renesas, Anson.Huang, masahiroy, michael, krzk, linux-kernel, vkoul, olof, vincenzo.frascino, ardb, linux-arm-kernel > 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-01 14:25 ` wangrongwei @ 2020-12-01 15:37 ` Marc Zyngier 2020-12-03 5:45 ` Rongwei Wang 0 siblings, 1 reply; 28+ messages in thread From: Marc Zyngier @ 2020-12-01 15:37 UTC (permalink / raw) To: wangrongwei Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, gshan, geert+renesas, Anson.Huang, masahiroy, michael, krzk, linux-kernel, vkoul, olof, vincenzo.frascino, ardb, linux-arm-kernel 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... ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-01 15:37 ` Marc Zyngier @ 2020-12-03 5:45 ` Rongwei Wang 2020-12-03 8:35 ` Marc Zyngier 0 siblings, 1 reply; 28+ messages in thread From: Rongwei Wang @ 2020-12-03 5:45 UTC (permalink / raw) To: Marc Zyngier Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, gshan, geert+renesas, Anson.Huang, masahiroy, michael, krzk, linux-kernel, vkoul, olof, vincenzo.frascino, ardb, linux-arm-kernel > 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-03 5:45 ` Rongwei Wang @ 2020-12-03 8:35 ` Marc Zyngier 2020-12-03 11:25 ` Rongwei Wang 0 siblings, 1 reply; 28+ messages in thread From: Marc Zyngier @ 2020-12-03 8:35 UTC (permalink / raw) To: Rongwei Wang Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, gshan, geert+renesas, Anson.Huang, masahiroy, michael, krzk, linux-kernel, vkoul, olof, vincenzo.frascino, ardb, linux-arm-kernel 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... ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-03 8:35 ` Marc Zyngier @ 2020-12-03 11:25 ` Rongwei Wang 2020-12-03 11:45 ` Marc Zyngier 2020-12-03 11:50 ` Mark Rutland 0 siblings, 2 replies; 28+ messages in thread From: Rongwei Wang @ 2020-12-03 11:25 UTC (permalink / raw) To: Marc Zyngier Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, gshan, geert+renesas, Anson.Huang, masahiroy, michael, krzk, linux-kernel, vkoul, olof, vincenzo.frascino, ardb, linux-arm-kernel > 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-03 11:25 ` Rongwei Wang @ 2020-12-03 11:45 ` Marc Zyngier 2020-12-03 12:22 ` Rongwei Wang 2020-12-03 11:50 ` Mark Rutland 1 sibling, 1 reply; 28+ messages in thread From: Marc Zyngier @ 2020-12-03 11:45 UTC (permalink / raw) To: Rongwei Wang Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, gshan, geert+renesas, Anson.Huang, masahiroy, michael, krzk, linux-kernel, vkoul, olof, vincenzo.frascino, ardb, linux-arm-kernel 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... ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-03 11:45 ` Marc Zyngier @ 2020-12-03 12:22 ` Rongwei Wang 0 siblings, 0 replies; 28+ messages in thread From: Rongwei Wang @ 2020-12-03 12:22 UTC (permalink / raw) To: Marc Zyngier Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, gshan, geert+renesas, Anson.Huang, masahiroy, michael, krzk, linux-kernel, vkoul, olof, vincenzo.frascino, ardb, linux-arm-kernel > 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-03 11:25 ` Rongwei Wang 2020-12-03 11:45 ` Marc Zyngier @ 2020-12-03 11:50 ` Mark Rutland 1 sibling, 0 replies; 28+ messages in thread From: Mark Rutland @ 2020-12-03 11:50 UTC (permalink / raw) To: Rongwei Wang Cc: Marc Zyngier, vkoul, gshan, Anson.Huang, geert+renesas, catalin.marinas, masahiroy, linux-kernel, krzk, bjorn.andersson, michael, olof, shawnguo, vincenzo.frascino, Will Deacon, ardb, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-11-30 17:48 [PATCH 0/3] arm64:msr: Add MSR driver Rongwei Wang ` (4 preceding siblings ...) 2020-11-30 18:05 ` Marc Zyngier @ 2020-11-30 19:03 ` Borislav Petkov 2020-12-01 3:44 ` wangrongwei 5 siblings, 1 reply; 28+ messages in thread From: Borislav Petkov @ 2020-11-30 19:03 UTC (permalink / raw) To: Rongwei Wang Cc: catalin.marinas, will, bjorn.andersson, shawnguo, vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-11-30 19:03 ` Borislav Petkov @ 2020-12-01 3:44 ` wangrongwei 2020-12-01 11:26 ` Borislav Petkov 0 siblings, 1 reply; 28+ messages in thread From: wangrongwei @ 2020-12-01 3:44 UTC (permalink / raw) To: Borislav Petkov Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-01 3:44 ` wangrongwei @ 2020-12-01 11:26 ` Borislav Petkov 2020-12-01 14:33 ` wangrongwei 0 siblings, 1 reply; 28+ messages in thread From: Borislav Petkov @ 2020-12-01 11:26 UTC (permalink / raw) To: wangrongwei Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-01 11:26 ` Borislav Petkov @ 2020-12-01 14:33 ` wangrongwei 2020-12-01 14:54 ` Borislav Petkov 0 siblings, 1 reply; 28+ messages in thread From: wangrongwei @ 2020-12-01 14:33 UTC (permalink / raw) To: Borislav Petkov Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel > 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-01 14:33 ` wangrongwei @ 2020-12-01 14:54 ` Borislav Petkov 2020-12-01 15:17 ` Rongwei Wang 0 siblings, 1 reply; 28+ messages in thread From: Borislav Petkov @ 2020-12-01 14:54 UTC (permalink / raw) To: wangrongwei Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-01 14:54 ` Borislav Petkov @ 2020-12-01 15:17 ` Rongwei Wang 2020-12-01 15:25 ` Borislav Petkov 2020-12-01 15:26 ` Ard Biesheuvel 0 siblings, 2 replies; 28+ messages in thread From: Rongwei Wang @ 2020-12-01 15:17 UTC (permalink / raw) To: Borislav Petkov Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel > 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-01 15:17 ` Rongwei Wang @ 2020-12-01 15:25 ` Borislav Petkov 2020-12-03 2:09 ` Rongwei Wang 2020-12-01 15:26 ` Ard Biesheuvel 1 sibling, 1 reply; 28+ messages in thread From: Borislav Petkov @ 2020-12-01 15:25 UTC (permalink / raw) To: Rongwei Wang Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-01 15:25 ` Borislav Petkov @ 2020-12-03 2:09 ` Rongwei Wang 0 siblings, 0 replies; 28+ messages in thread From: Rongwei Wang @ 2020-12-03 2:09 UTC (permalink / raw) To: Borislav Petkov Cc: catalin.marinas, Will Deacon, bjorn.andersson, shawnguo, vkoul, geert+renesas, Anson.Huang, michael, krzk, olof, vincenzo.frascino, ardb, masahiroy, gshan, linux-arm-kernel, linux-kernel > 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/3] arm64:msr: Add MSR driver 2020-12-01 15:17 ` Rongwei Wang 2020-12-01 15:25 ` Borislav Petkov @ 2020-12-01 15:26 ` Ard Biesheuvel 1 sibling, 0 replies; 28+ messages in thread From: Ard Biesheuvel @ 2020-12-01 15:26 UTC (permalink / raw) To: Rongwei Wang Cc: Borislav Petkov, Catalin Marinas, Will Deacon, Bjorn Andersson, shawnguo, vkoul, Geert Uytterhoeven, Anson.Huang, michael, Krzysztof Kozlowski, Olof Johansson, Vincenzo Frascino, Masahiro Yamada, gshan, Linux ARM, Linux Kernel Mailing List 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2020-12-03 12:23 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-30 17:48 [PATCH 0/3] arm64:msr: Add MSR driver Rongwei Wang 2020-11-30 17:48 ` [PATCH 1/3] arm64:insn: Export symbols for MSR ARM driver Rongwei Wang 2020-11-30 17:48 ` [PATCH 2/3] arm64:msr: Introduce " Rongwei Wang 2020-11-30 17:48 ` [PATCH 3/3] arm64:msr: Enable " Rongwei Wang 2020-11-30 17:57 ` [PATCH 0/3] arm64:msr: Add MSR driver Will Deacon 2020-12-01 2:55 ` wangrongwei 2020-12-01 5:44 ` wangrongwei 2020-11-30 18:05 ` Marc Zyngier 2020-11-30 18:20 ` Randy Dunlap 2020-12-01 3:09 ` wangrongwei 2020-12-01 8:12 ` Marc Zyngier 2020-12-01 14:25 ` wangrongwei 2020-12-01 15:37 ` Marc Zyngier 2020-12-03 5:45 ` Rongwei Wang 2020-12-03 8:35 ` Marc Zyngier 2020-12-03 11:25 ` Rongwei Wang 2020-12-03 11:45 ` Marc Zyngier 2020-12-03 12:22 ` Rongwei Wang 2020-12-03 11:50 ` Mark Rutland 2020-11-30 19:03 ` Borislav Petkov 2020-12-01 3:44 ` wangrongwei 2020-12-01 11:26 ` Borislav Petkov 2020-12-01 14:33 ` wangrongwei 2020-12-01 14:54 ` Borislav Petkov 2020-12-01 15:17 ` Rongwei Wang 2020-12-01 15:25 ` Borislav Petkov 2020-12-03 2:09 ` Rongwei Wang 2020-12-01 15:26 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).