On 2 Feb 2023, at 21:00, Jessica Clarke <jrt...@freebsd.org> wrote:
>
> On 2 Feb 2023, at 16:48, Andrew Turner <and...@freebsd.org> wrote:
>>
>> The branch main has been updated by andrew:
>>
>> URL:
>> https://cgit.FreeBSD.org/src/commit/?id=f29942229d24ebb8b98f8c5d02f3c8632648007e
>>
>> commit f29942229d24ebb8b98f8c5d02f3c8632648007e
>> Author: Andrew Turner <and...@freebsd.org>
>> AuthorDate: 2023-01-25 17:47:39 +0000
>> Commit: Andrew Turner <and...@freebsd.org>
>> CommitDate: 2023-02-02 16:43:15 +0000
>>
>> Read the arm64 far early in el0 exceptions
>>
>> 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@
>> Reviewed by: markj
>> Sponsored by: Arm Ltd
>> Differential Revision: https://reviews.freebsd.org/D38196
>> ---
>> sys/arm64/arm64/exception.S | 15 +++++++++++++++
>> sys/arm64/arm64/trap.c | 26 +++++++-------------------
>> 2 files changed, 22 insertions(+), 19 deletions(-)
>>
>> diff --git a/sys/arm64/arm64/exception.S b/sys/arm64/arm64/exception.S
>> index 4a74358afeb9..55bac5e5228a 100644
>> --- a/sys/arm64/arm64/exception.S
>> +++ b/sys/arm64/arm64/exception.S
>> @@ -212,10 +212,25 @@ ENTRY(handle_el1h_irq)
>> END(handle_el1h_irq)
>>
>> ENTRY(handle_el0_sync)
>> + /*
>> + * Read the fault address early. The current thread structure may
>> + * be transiently unmapped if it is part of a memory range being
>> + * promoted or demoted to/from a superpage. As this involves a
>> + * break-before-make sequence there is a short period of time where
>> + * an access will raise an exception. If this happens the fault
>> + * address will be changed to the kernel address so a later read of
>> + * far_el1 will give the wrong value.
>> + *
>> + * The earliest memory access that could trigger a fault is in a
>> + * function called by the save_registers macro so this is the latest
>> + * we can read the userspace value.
>> + */
>> + mrs x19, far_el1
>> save_registers 0
>> ldr x0, [x18, #PC_CURTHREAD]
>> mov x1, sp
>> str x1, [x0, #TD_FRAME]
>> + mov x2, x19
>> bl do_el0_sync
>> do_ast
>> restore_registers 0
>> diff --git a/sys/arm64/arm64/trap.c b/sys/arm64/arm64/trap.c
>> index 4e54a06548cc..1b33d7aa60c4 100644
>> --- a/sys/arm64/arm64/trap.c
>> +++ b/sys/arm64/arm64/trap.c
>> @@ -76,7 +76,7 @@ __FBSDID("$FreeBSD$");
>>
>> /* Called from exception.S */
>> void do_el1h_sync(struct thread *, struct trapframe *);
>
> This did not address my feedback regarding EL1 debug exceptions also
> clobbering FAR.
Ping, now after this has been MFC’ed without so much as a reply to my
feedback here nor on the Phabricator review.
Jess
>> -void do_el0_sync(struct thread *, struct trapframe *);
>> +void do_el0_sync(struct thread *, struct trapframe *, uint64_t far);
>> void do_el0_error(struct trapframe *);
>> void do_serror(struct trapframe *);
>> void unhandled_exception(struct trapframe *);
>> @@ -559,11 +559,11 @@ do_el1h_sync(struct thread *td, struct trapframe
>> *frame)
>> }
>>
>> void
>> -do_el0_sync(struct thread *td, struct trapframe *frame)
>> +do_el0_sync(struct thread *td, struct trapframe *frame, uint64_t far)
>> {
>> pcpu_bp_harden bp_harden;
>> uint32_t exception;
>> - uint64_t esr, far;
>> + uint64_t esr;
>> int dfsc;
>>
>> /* Check we have a sane environment when entering from userland */
>> @@ -573,27 +573,15 @@ do_el0_sync(struct thread *td, struct trapframe *frame)
>>
>> esr = frame->tf_esr;
>> exception = ESR_ELx_EXCEPTION(esr);
>> - switch (exception) {
>> - case EXCP_INSN_ABORT_L:
>> - far = READ_SPECIALREG(far_el1);
>> -
>> + if (exception == EXCP_INSN_ABORT_L && far > VM_MAXUSER_ADDRESS) {
>> /*
>> * Userspace may be trying to train the branch predictor to
>> * attack the kernel. If we are on a CPU affected by this
>> * call the handler to clear the branch predictor state.
>> */
>> - if (far > VM_MAXUSER_ADDRESS) {
>> - bp_harden = PCPU_GET(bp_harden);
>> - if (bp_harden != NULL)
>> - bp_harden();
>> - }
>> - break;
>> - case EXCP_UNKNOWN:
>> - case EXCP_DATA_ABORT_L:
>> - case EXCP_DATA_ABORT:
>> - case EXCP_WATCHPT_EL0:
>> - far = READ_SPECIALREG(far_el1);
>> - break;
>> + bp_harden = PCPU_GET(bp_harden);
>> + if (bp_harden != NULL)
>> + bp_harden();
>> }
>> intr_enable();
