Read the arm64 far early in el0 exceptions

[andrew]

Summary

When handling userspace exceptions on arm64 we need to dereference the
current thread pointer. If this is being promoted/demoted there is a
small window where it will cause another exception to be hit. As this
second exception will set the fault address register we will read the
incorrect value in the userspace exception handler.

Fix this be always reading the fault address before dereferencing the
current thread pointer.

Reported by: olivier@
Sponsored by: Arm Ltd

[jrtc27]

sys/arm64/arm64/exception.S

215

This is dodgy, this only works because save_registers has been optimised with save_registers_head split out. Without that optimisation you'd end up clobbering userspace's x2.

Better to put this after save_registers instead (but still before any dereferencing of pc_curthread to save td_frame).

sys/arm64/arm64/trap.c

79

Why not just put it in the trapframe like esr? Having the extra argument is a bit ugly. Besides, do_el1h_sync calls into dtrace before reading far, even if the assembly that calls it doesn't access the current thread, so you can still end up faulting and clobbering far before you read it for from-EL1 exceptions.

[markj]

sys/arm64/arm64/exception.S

219

I'd add a comment explaining that the FAR has to be loaded before this store, as it's not particularly obvious why.

[andrew]

sys/arm64/arm64/exception.S

215

It's there because we dereference curthread in save_registers in the calls to ptrauth_exit_el0 and dbg_monitor_enter. save_registers should probably be renamed, but that's out of scope for this change.

219

I found it because of the dereference in dbg_monitor_enter so will add a comment as it's not obvious where the first dereference is without a lot of digging.

sys/arm64/arm64/trap.c

79

We could move reading the fault address earlier in the EL1 handler, but I don't see any places we could raise a data abort.

We are already careful to not dereference curthread as we may be handling it being unmepped.

The call into dtrace is to dtrace_trap. If this faults we have bigger issues as it could cause the exception handler to fault, etc.

[jrtc27]

sys/arm64/arm64/trap.c

79

Watchpoints hit during do_el1h_sync will clobber it, though (and the code is careful to permit that by not unmasking watchpoints when processing an exception from one). I really do not like micro-optimising something that even amd64 doesn't optimise and having to do all this nuanced reasoning to debate whether or not it's correct. Shove it in the trapframe alongside esr and then you know it's going to be fine. Plus that lets ddb print out the faulting address in backtraces, something which riscv does and amd64 could do but arm64 cannot.

[olivier]

This fix all the error I had with simple make -j 16 clean.
I'm able to run make -j 160 buildworld now without any core generated.

[andrew]

• Add a comment explaining why we store far early
• Use a callee saved register for far so we don't trash it
• Replace a switch statement with one case with an if statement

[andrew]

My plan is to merge this to stable/13. For this I'd prefer to not change the trap frame as that has a higher risk to get correct & could be consifered a KBI change.

[markj]

sys/arm64/arm64/exception.S

217