Page MenuHomeFreeBSD

riscv: handle page faults in the unmappable region
Needs ReviewPublic

Authored by mhorne on Sat, Jul 17, 7:50 PM.

Details

Reviewers
markj
jhb
jrtc27
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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 40512
Build 37401: arc lint + arc unit

Event Timeline

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?

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.

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?