Jonathan Neuschäfer has posted comments on this change. ( https://review.coreboot.org/28394 )
Change subject: riscv: Fix the definition of DEFINE_MPRV_READ ......................................................................
Patch Set 1: Code-Review-1
(2 comments)
I think the commit in its current form creates a (security) problem
https://review.coreboot.org/#/c/28394/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/28394/1//COMMIT_MSG@10 PS1, Line 10: So make this change. As far as I understand it, unprivileged code could use this to read from execute-only pages by reading at misaligned addresses and triggering the misaligned memory access handler. To avoid this problem, MXR should probably only be used to read instructions, but not to read data.
https://review.coreboot.org/#/c/28394/1/src/arch/riscv/include/vm.h File src/arch/riscv/include/vm.h:
https://review.coreboot.org/#/c/28394/1/src/arch/riscv/include/vm.h@41 PS1, Line 41: #define DEFINE_MPRV_READ(name, type, insn) \ I suggest the following:
- Rename DEFINE_MPRV_READ to DEFINE_MPRV_READ_FLAGS and add an extra parameter called "flags" which is OR'd to the mprv variable in the definition. - #define DEFINE_MPRV_READ(name, type, insn) DEFINE_MPRV_READ_FLAGS(name, type, insn, 0) - #define DEFINE_MPRV_READ_MXR(name, type, insn) DEFINE_MPRV_READ_FLAGS(name, type, insn, MSTATUS_MXR) - Add a separate MXR-enabled read function for bytes.