Bug 265439
| Summary: | copyin() repeatedly traps on some illegal user addresses on RISC-V | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Robert Morris <rtm> |
| Component: | riscv | Assignee: | Mark Johnston <markj> |
| Status: | Closed FIXED | ||
| Severity: | Affects Some People | CC: | jrtc27 |
| Priority: | --- | ||
| Version: | CURRENT | ||
| Hardware: | riscv | ||
| OS: | Any | ||
This is a regression in 31218f3209ac ("riscv: Add support for enabling SV48 mode"), right? We do check VIRT_IS_VALID in page_fault_handler and will SIGSEGV the process or panic the kernel depending on which mode faulted, and copyin etc will check the address is for userspace before using it, but they check VM_MAX(_)USER_ADDRESS which is now the SV48 version even when SV39 is in use.
Here's a patch which fixes the problem for me: https://reviews.freebsd.org/D35952 (In reply to Mark Johnston from comment #2) This patch eliminates the problem for me as well. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=828ea49debe34fddf63cb648b9e57871a34158b6 commit 828ea49debe34fddf63cb648b9e57871a34158b6 Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2022-07-28 13:38:52 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2022-07-28 18:33:39 +0000 riscv: Avoid passing invalid addresses to pmap_fault() After the addition of SV48 support, VIRT_IS_VALID() did not exclude addresses that are in the SV39 address space hole but not in the SV48 address space hole. This can result in mishandling of accesses to that range when in SV39 mode. Fix the problem by modifying VIRT_IS_VALID() to use the runtime address space bounds. Then, if the address is invalid, and pcb_onfault is set, give vm_fault_trap() a chance to veto the access instead of panicking. PR: 265439 Reviewed by: jhb Reported and tested by: Robert Morris <rtm@lcs.mit.edu> Fixes: 31218f3209ac ("riscv: Add support for enabling SV48 mode") MFC after: 1 week Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D35952 sys/riscv/include/pmap.h | 5 +++++ sys/riscv/include/vmparam.h | 4 ---- sys/riscv/riscv/pmap.c | 2 ++ sys/riscv/riscv/trap.c | 7 ++----- 4 files changed, 9 insertions(+), 9 deletions(-) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=2f1cdb1e4883a962ff305f7422495122516983df commit 2f1cdb1e4883a962ff305f7422495122516983df Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2022-07-28 13:38:52 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2022-08-04 13:57:15 +0000 riscv: Avoid passing invalid addresses to pmap_fault() After the addition of SV48 support, VIRT_IS_VALID() did not exclude addresses that are in the SV39 address space hole but not in the SV48 address space hole. This can result in mishandling of accesses to that range when in SV39 mode. Fix the problem by modifying VIRT_IS_VALID() to use the runtime address space bounds. Then, if the address is invalid, and pcb_onfault is set, give vm_fault_trap() a chance to veto the access instead of panicking. PR: 265439 Reviewed by: jhb Reported and tested by: Robert Morris <rtm@lcs.mit.edu> Fixes: 31218f3209ac ("riscv: Add support for enabling SV48 mode") Sponsored by: The FreeBSD Foundation (cherry picked from commit 828ea49debe34fddf63cb648b9e57871a34158b6) sys/riscv/include/pmap.h | 5 +++++ sys/riscv/include/vmparam.h | 4 ---- sys/riscv/riscv/pmap.c | 2 ++ sys/riscv/riscv/trap.c | 7 ++----- 4 files changed, 9 insertions(+), 9 deletions(-) Thank you for the report. |
FreeBSD-CURRENT on qemu 6.2.0's riscv64 emulation can disagree with the "hardware" about whether upper bits of SV39 virtual addresses are significant. copyin() will get a page fault from the hardware if a user-supplied address has a few bits higher than the 39th set, but the pmap.c pmap_xx_index() macros ignore those high bits, so pmap_fault() may treat it as a valid user address. So the trap may return to copyin(), which will fault again on the same address... Here's a program that does that for me. int main() { char buf[512]; write(1, 0x500000000000ULL | (unsigned long) buf, 1); } Here's a typical ddb backtrace: pmap_fault() at pmap_fault+0xc0 page_fault_handler() at page_fault_handler+0x11c do_trap_supervisor() at do_trap_supervisor+0x76 cpu_exception_handler_supervisor() at cpu_exception_handler_supervisor+0x70 --- exception 13, tval = 0x500080e1f230 copyin() at copyin+0x68 uiomove() at uiomove+0xe log_console() at log_console+0x60 ttyconsdev_write() at ttyconsdev_write+0x1a devfs_write_f() at devfs_write_f+0xa6 fo_write() at fo_write+0xa dofilewrite() at dofilewrite+0x66 kern_writev() at kern_writev+0x40 sys_write() at sys_write+0x54 syscallenter() at syscallenter+0xec ecall_handler() at ecall_handler+0x18 do_trap_user() at do_trap_user+0xea cpu_exception_handler_user() at cpu_exception_handler_user+0x72