Bug 265439 - copyin() repeatedly traps on some illegal user addresses on RISC-V
Summary: copyin() repeatedly traps on some illegal user addresses on RISC-V
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: riscv (show other bugs)
Version: CURRENT
Hardware: riscv Any
: --- Affects Some People
Assignee: Mark Johnston
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-07-25 23:05 UTC by Robert Morris
Modified: 2022-08-04 13:58 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Morris 2022-07-25 23:05:50 UTC
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
Comment 1 Jessica Clarke freebsd_committer freebsd_triage 2022-07-25 23:27:40 UTC
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.
Comment 2 Mark Johnston freebsd_committer freebsd_triage 2022-07-26 17:49:39 UTC
Here's a patch which fixes the problem for me: https://reviews.freebsd.org/D35952
Comment 3 Robert Morris 2022-07-28 09:18:28 UTC
(In reply to Mark Johnston from comment #2)
This patch eliminates the problem for me as well.
Comment 4 commit-hook freebsd_committer freebsd_triage 2022-07-28 19:04:09 UTC
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(-)
Comment 5 commit-hook freebsd_committer freebsd_triage 2022-08-04 13:57:48 UTC
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(-)
Comment 6 Mark Johnston freebsd_committer freebsd_triage 2022-08-04 13:58:57 UTC
Thank you for the report.