riscv: handle page faults in the unmappable region

[mhorne]

Summary

When handling a kernel page fault, check explicitly that stval resides
in either the user or kernel address spaces, and make the page fault
fatal if not. Otherwise, a properly crafted address may appear to
pmap_fault() as a valid and present page in the kernel map, causing the
page fault to be retried continuously. This is mainly due to the fact
that the upper bits of virtual addresses are not validated by most of
the pmap code.

Faults of this nature should only occur due to some kind of bug in the
kernel, but it is best to handle them gracefully when they do.

Test Plan

This issue can be triggered by the test cases presented in PR 257193. With this change, these are upgraded to a fatal page fault panic.

[jrtc27]

This will eventually need to be a dynamic property based on the page table format in use. Would it not be better to just make pmap_fault verify that the address is suitably sign-extended and return 0 if not?

[mhorne]

In D31208#702538, @jrtc27 wrote:

This will eventually need to be a dynamic property based on the page table format in use. Would it not be better to just make pmap_fault verify that the address is suitably sign-extended and return 0 if not?

I mostly prefer the explicitness in this approach, but I could be convinced otherwise. Is your concern about performance?

In my mind, we would #define VM_MIN_KERNEL_ADDRESS VM_MIN_KERNEL_ADDRESS_SV48 when we add that support, even when running sv39. This isn't the only check against these VM_...ADDRESS values, so I don't think this change introduces any new challenges in that regard.

I was thinking we should add an assertion for the sign-extension to pmap_l1(), which should catch most violations. Something similar to D31179. While this would also catch the issue presented here, I don't think it's wrong to handle this case specifically.

[jrtc27]

Hm. it was mainly based on the fact that the if (stval >= VM_MAX_USER_ADDRESS) { } else { } pattern is common to a lot of architectures whereas this way of writing it is not. Though arm64 for example puts the logic in the trap handler rather than the pmap too by virtue of checking ADDR_IS_CANONICAL first, then using code that mirrors what we currently have for riscv. I quite like that separation of separating out the validity check from the address space check. But perhaps pmap_fault could still benefit from a KASSERT that the passed address is valid for the current translation mode?

[mhorne]

Handle jrtc27's suggestion, add VIRT_IS_VALID() to vmparam.h.

Add an assertion to pmap_l1().

[markj]

sys/riscv/riscv/pmap.c

355

No need for newlines in kassert/panic strings.

sys/riscv/riscv/trap.c

217

In the user mode path, we call pmap_fault() without validating the address. Doesn't that mean that the assertion in pmap_l1() can be tripped from user mode?

[mhorne]

Handle !VIRT_IS_VALID for the usermode case.

Remove extra \n from KASSERT.

[mhorne]

sys/riscv/riscv/trap.c

217

Yeah... good catch there, thanks.

[jrtc27]

sys/riscv/riscv/pmap.c

355

for my sanity, please :) (one of the things I loathe about the arm64 port...)

[mhorne]

Add the hex prefix.